You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by greedy <gi...@git.apache.org> on 2016/04/07 06:08:07 UTC
[GitHub] incubator-tinkerpop pull request: Fix TINKERPOP-1252
GitHub user greedy opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/288
Fix TINKERPOP-1252
It is now ensured that if there was any problem committing or rolling
back a transaction then the ThreadLocal binding to a Neo4j transaction
will be cleared.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/greedy/incubator-tinkerpop TINKERPOP-1252
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/288.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #288
----
commit 900e9b59ac2cd0d6e4acead65fed84102a488b84
Author: Geoff Reedy <ge...@programmer-monk.net>
Date: 2016-04-07T04:02:12Z
Fix TINKERPOP-1252
It is now ensured that if there was any problem committing or rolling
back a transaction then the ThreadLocal binding to a Neo4j transaction
will be cleared.
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Fix TINKERPOP-1252
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/288#issuecomment-206863431
> 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's not pretty, but I think you should. In that way, we can be sure everything gets called and if neo4j ever changes that code to provide a way to fail, we'll be covered.
> I wasn't sure if I should create two pull requests: this one and one for the tp31 branch
Please just re-target this PR to tp31 and we'll handling merging it appropriately. Thanks
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Fix TINKERPOP-1252
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/288#issuecomment-206806234
So you're saying that there is some case where `threadLocalTx.get().close();` fails in the `finally` clause and then we don't get a nice call to `threadLocalTx.remove();`. Doesn't your change introduce a new problem though, where a fail on a call to `threadLocalTx.get().success();` or `threadLocalTx.get().failure();` would mean that `threadLocalTx.get().close();` would never get called?
Also, is this only a problem for 3.2.0? seems like this fix should be targetted at 3.1.2 and the tp31 branch, no?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Fix TINKERPOP-1252
Posted by greedy <gi...@git.apache.org>.
Github user greedy commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/288#issuecomment-207040134
Closing this PR to open a new one targeting tp31
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Fix TINKERPOP-1252
Posted by greedy <gi...@git.apache.org>.
Github user greedy closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/288
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Fix TINKERPOP-1252
Posted by greedy <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---