Github user greedy commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/288#issuecomment-206860135
Yeah, that's what I saw happening. threadLocalTx.remove() was skipped
because threadLocalTx.get().close() failed.
I did think about using two levels of finally blocks to guarantee that
close is called. The Neo4j API documentation doesn't really say much about
which of these methods can fail and in which ways. I figured that if either one
of those fail then the transaction should be considered dead. From looking
through the implementations when I was trying to figure out what was going on,
I saw that success() and failure() just set a boolean flag so they shouldn't
fail. I wouldn't have any problem with cascaded finally blocks to make sure
everything is getting called; if that seems better to you, I can update the
pull request to do that.
It does indeed impact the 3.1 series as that is where I actually
encountered it. It should be applied to both so I marked both series as
affected in the JIRA issue. I wasn't sure if I should create two pull requests:
this one and one for the tp31 branch; or if a committer would just cherry-pick
this fix on to tp31 if/when it lands on master.
I did run the neo4j test suite for the change and they passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
