eXist-db / exist

eXist Native XML Database and Application Platform
https://exist-db.org
GNU Lesser General Public License v2.1
416 stars 179 forks source link

[BUG] resource leak on Ubuntu 22.04 #5345

Open line-o opened 2 weeks ago

line-o commented 2 weeks ago

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

Additional context

adamretter commented 2 weeks ago

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

StephanMa commented 2 weeks ago

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

line-o commented 2 weeks ago

Yes, JDKs are the same.

dizzzz commented 2 days ago

@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 commented 2 hours ago

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.