[
https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12899483#action_12899483
]
Bryan Duxbury commented on THRIFT-845:
--------------------------------------
Some thoughts:
-Could we move the tracking of last action into TAsyncMethodCall? I'm wary of
putting hashmap accesses into such a sensitive loop.
-I'd prefer if you stored long milliseconds since epoch instead of Date
objects. We don't need the Date abstraction.
-The catch blocks in startPendingMethodCalls seem very redundant. I think it
would make more sense to just reduce it to a single catch (Throwable t) block.
Also, you should log the exception before you call the onError, since there's
the possibility that onError could fail for some reason.
Otherwise, this is a great patch, and the tests pass for me. With the slight
revisions above, I'll be happy to commit this.
> async client does not respect timeout
> -------------------------------------
>
> Key: THRIFT-845
> URL: https://issues.apache.org/jira/browse/THRIFT-845
> Project: Thrift
> Issue Type: Bug
> Components: Library (Java)
> Affects Versions: 0.4, 0.5
> Reporter: Eric Jensen
> Fix For: 0.5
>
> Attachments: async_client_timeout.diff
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> there effectively is no timeout on calls via the async client, so the
> consumer may never be called back with either a success or error. this is
> not an awesome contract, and it also leaves the potential for the client
> itself to leak. ning is working on a patch for this based on the below
> discussion:
> implementing the timeout inside TAsyncClientManager seems pretty easy, we
> would just loop through Selector.keys() there and make sure none of them have
> been waiting for longer than the SO_TIMEOUT we set on its TNonblockingSocket,
> if one is, we just do methodCall.onError on it to make sure the caller is
> notified.
> We also need to change Selector.select() to Selector.select(long timeout),
> with a small value so that the selector thread wakes up with sufficient
> granularity to timeout calls.
> I think we can just track the timeout of the pending operation within
> TAsyncMethodCall itself and loop over Selector.keys() to check them. But
> since we just made the change to leave the keys for all open connections
> registered with the selector until they are closed, this could be somewhat
> costly.
> For reference, here's how jetty does this
> (http://grepcode.com/file/repo1.maven.org/maven2/org.eclipse.jetty/jetty-io/8.0.0.M0/org/eclipse/jetty/io/nio/SelectorManager.java#SelectorManager)
> which i do _not_ encourage you to read, but i am confident from reading it
> that it's doing just what i said: tracking the list of outstanding
> read/write operations and their timeouts themselves, looping through them
> within the selector thread, and closing their sockets if their so-called
> "idle timeout" has expired.
> Also, there are a couple java selector bugs jetty links to that we should be
> aware of, the only one applicable to us has been fixed in jdk 1.6 update 18
> (the latest is 21).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
