GH-1958: fix resource closing in federated service SilentIteration #1959
Conversation
| try { | ||
| Iterations.closeCloseable(iter); | ||
| } finally { | ||
| super.handleClose(); | ||
| } |
There was a problem hiding this comment.
Should maybe this also fail silently?
| @@ -34,9 +39,20 @@ protected BindingSet getNextElement() throws QueryEvaluationException { | |||
| return iter.next(); | |||
There was a problem hiding this comment.
I know it's not required by our styleguide, but I do prefer using { } for if/else/while.
There was a problem hiding this comment.
Agreed. Actually it is a convention for the RDF4J project, it's just not explicitly enforced by the formatter because in Eclipse, this option is not part of the formatting rules (so I can't make it part of the maven formatter plugin, which uses Eclipse formatter under the hood).
Btw if you're using Eclipse, you can configure it to automatically add braces on save, by going to 'Preferences' -> 'Editor' -> 'Save Actions'. Enable 'Additional actions', click 'Configure...' and in the 'Code Style' dialog enable the option 'Use blocks in if/while/for/do statements'.
Or if you prefer you can use an Eclipse clean up profile to do the same thing.
| @@ -19,6 +22,8 @@ | |||
| */ | |||
| public class SilentIteration extends LookAheadIteration<BindingSet, QueryEvaluationException> { | |||
There was a problem hiding this comment.
I only just noticed this after pushing an editorial fix, but apparently we have two SilentIteration classes:
org.eclipse.rdf4j.repository.sparql.federation.SilentIterationorg.eclipse.rdf4j.query.algebra.evaluation.iterator.SilentIteration
They look identical (barring the fixes you just did) - do we need both or can we get rid of the federated one?
There was a problem hiding this comment.
Ah - to answer my own question, this is probably a deliberate copy to avoid a cyclic dependency.
Let's apply the fix to both copies in this PR. I'll look into a better solution so that we don't have to maintain separate copies separately.
There was a problem hiding this comment.
Looking a bit further, it seems the copy in evaluation.iterator is not actually used anywhere. Let's mark it deprecated, so we can decide to remove it in a later major release.
|
I will be pushing some fixes to this branch from my end - I need the practice to work with PRs from forked repos from my command line, and if all goes well I can clean this up quickly. |
There was a problem hiding this comment.
@aschwarte10 I've made some changes, basically by introducing a generic SilentIteration class in rdf4j-util and re-using that in the federation code. Let me know what you think.
LGTM, thanks for following up |
…tIteration The SilenetIteration failed to close the inner iteration, thus resulting in resource leaks (e.g. pending connections). Without this fix the attached unit tests takes 20s before forcefully closing the connection. Now it correctly terminates immediately after test execution. Note that additionally a TRACE logger has been introduced to give users the chance to observe errors. Additional changes: * add braces * apply fixes to both copies of SilentIteration * cleaned up different SilentIteration impls
aschwarte10
force-pushed
the
issues/#1958-silent-iteration-close
branch
from
f1c3097
to
783bc42
Compare
|
The commits are now squashed into a single one |
The SilenetIteration failed to close the inner iteration, thus resulting
in resource leaks (e.g. pending connections).
Without this fix the attached unit tests takes 20s before forcefully
closing the connection. Now it correctly terminates immediately after
test execution.
Note that additionally a TRACE logger has been introduced to give users
the chance to observe errors.
GitHub issue resolved: #1958