You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jsirois <gi...@git.apache.org> on 2016/02/08 21:07:21 UTC
[GitHub] thrift pull request: [Java] AsyncMethodCallback should be typed in...
GitHub user jsirois opened a pull request:
https://github.com/apache/thrift/pull/840
[Java] AsyncMethodCallback should be typed in generated AsyncIface
The parametrization brings the existing actual parametrization with
client call implementation objects to the fore and so this change
also fixes that parametrization to be a simple parametrization over
the return type as is done in the server-side AsyncProcessor code.
NB: This is a breaking change in both generated code and the client
libs.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jsirois/thrift THRIFT-3112
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/840.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 #840
----
----
---
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] thrift pull request: [Java] AsyncMethodCallback should be typed in...
Posted by jsirois <gi...@git.apache.org>.
Github user jsirois commented on the pull request:
https://github.com/apache/thrift/pull/840#issuecomment-181546218
NB: This is a straw-man at this point that picks up the work started in #500 by @mreve and whose merits is being discussed here: http://markmail.org/message/k77l6helvkh4igrv
Please do not merge pending results of that discussion - this is a breaking change for async client users.
---
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] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...
Posted by jsirois <gi...@git.apache.org>.
Github user jsirois commented on a diff in the pull request:
https://github.com/apache/thrift/pull/840#discussion_r53405396
--- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java ---
@@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key) {
key.interestOps(0);
// this ensures that the TAsyncMethod instance doesn't hang around
key.attach(null);
--- End diff --
Adding cancel won't (doesn't) work. The key is associated with the single underlying channel (wrapped in a `TNonblockingTransport`) from the `TAsyncClient` which is intended for use with multiple concurrent calls afaict. The `org.apache.thrift.async.TestTAsyncClientManager#testParallelCalls` test uses the client in this way - 1 client, multiple concurrent calls - and demonstrates that cancelling the key cancels the underlying channel and, for example, you get 1 successful call and 4999 failed calls in the 5000 call test case as a result.
---
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] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/840#discussion_r53421817
--- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java ---
@@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key) {
key.interestOps(0);
// this ensures that the TAsyncMethod instance doesn't hang around
key.attach(null);
--- End diff --
It turned out that it's one key per connection.
So indeed canceling here does not work at all, it spoils the connection after the first call.
Instead, cancel() should be called when the connection is closed but is not unless aborted by exceptions, so key objects are certainly leaking.
As it now seems a completely separate problem so I'll stop pulling it here and focus on this patch for now.
Can you explain as to why you are introducing try catch anyway ?
It's the last bit that stops me from going forward.
---
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] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/840#discussion_r53422103
--- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java ---
@@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key) {
key.interestOps(0);
// this ensures that the TAsyncMethod instance doesn't hang around
key.attach(null);
--- End diff --
Another implication of the current (pre-existing) design is that the client cannot handle unordered or even queued calls.
IMO it should but that would need major rewrite.
(To be clear, testParallelCalls has bunches of concurrent clients each of which does sequential calls)
---
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] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/840#discussion_r54286321
--- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java ---
@@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key) {
key.interestOps(0);
// this ensures that the TAsyncMethod instance doesn't hang around
key.attach(null);
--- End diff --
I started working on another async issue (THRIFT-3680) on top of your change.
As I actually ran your code, I think it immediately became evident that the try-catch is simply for new checked exception, period.
In that case, I'd like to put cancel() to the **catch** part to restore the original behavior.
Any concern for that ?
---
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] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...
Posted by jsirois <gi...@git.apache.org>.
Github user jsirois commented on a diff in the pull request:
https://github.com/apache/thrift/pull/840#discussion_r53405834
--- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java ---
@@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key) {
key.interestOps(0);
// this ensures that the TAsyncMethod instance doesn't hang around
key.attach(null);
--- End diff --
NB: I still don;t fully understand this code, but you've got me reading it more closely. I'll report back when I fully grok what's going on here in the pre-existing code.
---
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] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/840#discussion_r52841869
--- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java ---
@@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key) {
key.interestOps(0);
// this ensures that the TAsyncMethod instance doesn't hang around
key.attach(null);
--- End diff --
AFAIK, key is not removed from `selector.keys()` unless `key.cancel()` is called, so `interestOps(0)` and `attach(null)` is not enough (maybe wrong assumption ?).
Do you happen to have any idea why the original code does not call `cancel()` for the success path here ?
As your intention here seems to be to prevent `key.cancel()` in `transition()` for the error path, I thought you might know about it.
---
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] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/840#discussion_r52855940
--- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java ---
@@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key) {
key.interestOps(0);
// this ensures that the TAsyncMethod instance doesn't hang around
key.attach(null);
--- End diff --
maybe it's just a bug in the original code ?
Then I think we can simply add cancel() and stop key set from growing indefinitely without doing any harm at all.
What do you think ?
---
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] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/840
---
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.
---