Memory leak in higher-order functions

[wolfgangmm]

Most functions which take a function item as argument are leaking memory. This includes nearly all functions on arrays and maps, but also for-each and similar. The memory leak will at least result in higher memory consumption but frequently leads to a complete system breakdown on production servers facing higher traffic.

The PR does two things to fix the issue:

1. FunctionReference now implements Closable so the caller can make sure temporary objects are cleared after the function item was used. Existing code will continue to work, but all core functions have been changed accordingly.
2. above change leads to issues with closures in inline functions. Those should not be cleared until the very end of query execution. I have thus decoupled the lifecycle of closures, which makes sure we really clear all of them.

[adamretter]

@wolfgangmm looks very interesting...

So is it the case that a FunctionReference should always have close called on it? e.g. either implicitly through try-with-resources, or explicitly via #close()?
Additionally, should we always call close, or only if we create the FunctionReference and/or have ownership of it?

If that is the case. Do we not need to make further fixes to classes such as:

1. XQueryTestRunner
2. XMLTestRunner
3. ModuleFunctions
4. AbstractExtractFunction
5. ngram.HighlightMatches.MatchCallback
6. AdaptiveWriter
7. DynamicFunctionCall
8. PartialFunctionApplication
9. FunSort
10. CallFunction
11. IndexKeys
12. range.IndexKeys
13. PersistentLoginFunctions

[wolfgangmm]

@adamretter close (or resetState) should be called after a FunctionReference has been evaluated via eval or evalFunction, so any temporary object created during evaluation is cleaned up. It does not have to be called when the reference is just created or passed around. A function item can be long-lived as it may be passed around throughout a query. It is thus important to make sure resources are properly cleaned up.

I wrapped all occurrences I found into try-with-resource, but will recheck the ones you mentioned. I think I covered them all though.

[duncdrum]

@wolfgangmm does this mean we should update the code listing in Creating XQuery Modules?
eval(Sequence[] args, Sequence contextSequence)

[adamretter]

@wolfgangmm call let me know. I will then merge once you give me the "go ahead"

[joewiz]

NB: The appveyor failure was because the CI tests exceeded 60 minutes on one of the 4 tested environments, APPVEYOR_BUILD_WORKER_IMAGE=Ubuntu, JAVA_HOME=/usr/lib/jvm/java-10-openjdk-amd64. The other 3 tests passed.

[joewiz]

@wolfgangmm Thank you very much for your hard work in discovering this problem and fixing it. You mentioned on last week's call that your fix might require an API change, but I see that you've tagged this as targeting 4.2.0. I take it that you found a solution without an API change?

[wolfgangmm]

@joewiz the API change I first considered was to allow more than two states for Expression.resetState. In the end I found it makes more sense to decouple the lifecycle of closures because it is hard to predict if and when a function item might be used. This way we only need minor changes to how we use function items and existing code will not break.

[wolfgangmm]

@adamretter ok, I checked and found three more cases where FunctionReference.close should be called. If the checks pass it should be good to merge.

[dizzzz]

please check your indenting ? for future commits

[adamretter]

@wolfgangmm I have merged this because it is a definetly improvement. However, I have some concerns that there maybe yet more cases. How do we handle the case where a function reference is used for a variable declaration, and also for an external variable declaration?

For example in XSuite's org.exist.test.runner.XQueryTestRunner we create FunctionReferences, which are then evaluated many times by the XQuery code, but I am not certain if #close() is ever called on them? Whilst that is a XQuery -> Java example, I suspect there are cases where it is useful to declare a variable function reference in XQuery -> XQuery, e.g.

declare variable $local:some-compare-fun := function($a, $b) {
  $a gt $b
};

[wolfgangmm]

@adamretter #close() (or #resetState) only needs to be called if the FunctionReference is actually evaluated. Only the caller can know if it is done with using the FunctionReference or not. Your example of assigning a function item to a variable is quite common and heavily used in the code which revealed the memory leak. The variable declaration creates the inline function, but does not evaluate it. Only when the inline function is actually evaluated it needs to be cleared. I covered all XQuery expressions which operate on function items, so there's no possibility of missing some.

For Java code just make sure #close() is called after calls to FunctionReference#evalFunction or FunctionReference#eval. This can be done immediately or after the executing code fragment has completed.

[adamretter]

@wolfgangmm Thanks. Got it :-)