Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] resource leak on Ubuntu 22.04 #5345

Open
line-o opened this issue Jul 1, 2024 · 5 comments
Open

[BUG] resource leak on Ubuntu 22.04 #5345

line-o opened this issue Jul 1, 2024 · 5 comments
Labels
bug issue confirmed as bug
Milestone

Comments

@line-o
Copy link
Member

line-o commented Jul 1, 2024

Describe the bug

On a server running exist-db 6.2.0 on Ubuntu 22.04 the monitoring reported a constant growing number of open file handles. On further inspection it became apparent that these were all temporary files created by exist-db. These files would only be purged from the filesystem when exist-db was restarted. The same query executed on the same version of eXist-db running on Ubuntu 20.04 did not show this behaviour.

The smallest reproducible XQuery triggering this behaviour we found is:

"TEST" => util:string-to-binary() => compression:deflate() => response:stream-binary()

The above will create a temporary file in eXist-db's temporary file folder. The folder can be found by searching for a subfolder whose name starts with exist-db-temp-file-manager- followed by a number of digits. The parent folder can be queried with util:system-property("java.io.tmpdir")

The java implementation of util:string-to-binary might be the issue here as it is opening a stream (UnsynchronizedByteArrayInputStream) without using the form usually used throughout the exist-db codebase (try( ... ){ }).

protected BinaryValue stringToBinary(String str, String encoding) throws XPathException {
  try {
    return BinaryValueFromInputStream.getInstance(context, new Base64BinaryValueType(), new UnsynchronizedByteArrayInputStream(str.getBytes(encoding)), this);
  } catch(final UnsupportedEncodingException e) {
    throw new XPathException(this, "Unsupported encoding: " + encoding);
  }
}

Expected behavior

All temporary files associated with a query to be removed after the query has finished running.

To Reproduce

Leaking

  1. evaluate the test in eXide
    "TEST" => util:string-to-binary() => compression:deflate() => response:stream-binary()
  2. list the contents of the temporary directory
  3. there is one new temporary file
  4. it will remain until exist-db is restarted

Safe

"TEST" => util:base64-encode() => xs:base64Binary() => response:stream-binary()

and

reponse-stream-binary(compression:deflate(util:string-to-binary("TEST")))
  1. evaluate one of the safe examples above
  2. list the contents of the temporary directory
  3. there is no new temporary file

Context

  • Build: eXist-6.2.0
  • Java: openjdk version "1.8.0_312"
  • OS: Ubuntu 22.04.3 LTS

Additional context

  • How is eXist-db installed? built from source, running as a service
  • Any custom changes in e.g. conf.xml? several
@adamretter
Copy link
Contributor

I doubt it's an issue with Ubuntu. Is the JDK Version and vendor exactly the same on both versions of Ubuntu?

@StephanMa
Copy link
Contributor

We have this error since a couple of month which leads to running out of space

@line-o
Copy link
Member Author

line-o commented Jul 2, 2024

Yes, JDKs are the same.

@joewiz joewiz added the bug issue confirmed as bug label Jul 2, 2024
@joewiz joewiz added this to the eXist-6.2.1 milestone Jul 2, 2024
@dizzzz
Copy link
Member

dizzzz commented Jul 14, 2024

@line-o Are these ubuntu instances running 'on bare metal' of in a virtualized (docker, VMware, ...) environment ?

During the Monday-eve session you provided quite some details/convincing details, and that actually with a nice code cleanup (combined with try-resources) addresses the issue nicely.

@adamretter
Copy link
Contributor

adamretter commented Jul 17, 2024

The java implementation of util:string-to-binary might be the issue here as it is opening a stream (UnsynchronizedByteArrayInputStream) without using the form usually used throughout the exist-db codebase (try( ... ){ })

  1. I think this is unlikely as there is no file operation in the Java code snippet you presented. The code new UnsynchronizedByteArrayInputStream(str.getBytes(encoding)) simply reads from a byte array in-memory. In the worst case scenario - that memory will be released by the JVM GC when the UnsynchronizedByteArrayInputStream is no longer referenced. Also see the note below about ownership and cleanup.

  2. The implementation of binary value types (i.e. xs:base64Binary and xs:hexBinary) in eXist-db is very difficult as eXist-db does not have a Query Planner. The main concerns are three-fold fold and tightly inter-related:

    1. Lifetime - when will the value no longer be required. There have been many bugs in eXist-db in this area in eXist-db, each of which I have resolved when they were reported.
    2. Streaming and Immutability - In XQuery values are immutable and may be bound to multiple variables. In Java streams are ephemeral, i.e. once you read the stream the same stream cannot be read again. However we have to provide the idempotent view of multiple variable bindings over the same variable that XQuery demands over the ephemeral nature of Java streams. If the stream is long, or there are many streams, we cannot simply read it once and cache it in memory as we would quickly run out of memory. So we have various strategies for caching stream data, some of which involve writing a temporary file to disk.
    3. Ownership - who owns the value. The owner must release any resources associated with the value when its lifetime expires. Ideally the scope of the variable would be statically determined by a query planner, but eXist-db doesn't have a query planner. To ensure resources are released, at the moment each BinaryValue is recorded in the XQueryContext of the executing XQuery and should be released when the query finishes executing. I think there are a few early-release edge-cases implemented too for optimisation purposes, but these have proven notoriously difficult to get right as at execution time we often lack enough information to determine if a variable is going to be used further in the query.

I would suggest placing a breakpoint in the TemporaryFileManager so that you can see the call to create the temporary file in your simple query above, you can then use the stack trace to work backward and see what part of your query is creating it and why, you should then also check why the XQueryContext cleanup method is not called (it very much should be) to release the resources when the query finishes.

Also one final note, any changes to Temporary Files need to be thoroughly tested on Windows. The reason we have the TemporaryFileManager predominantly was to work around a load of previously reported and fixed bugs in the way that Windows handles Temporary Files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issue confirmed as bug
Projects
Status: Ready
Development

No branches or pull requests

5 participants