You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Eric Jensen (JIRA)" <ji...@apache.org> on 2010/08/10 22:58:20 UTC
[jira] Created: (THRIFT-845) async client does not respect timeout
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
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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-845:
---------------------------------
Fix Version/s: 0.5
> 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
>
> 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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12900124#action_12900124 ]
Bryan Duxbury commented on THRIFT-845:
--------------------------------------
bq. Only the select thread modifies the timeoutWatchSet - methods are entered when they are popped off the pending queue, not when they're pushed on.
Having threads involved is only one way to get a ConcurrentModification exception. Try this code out:
{code}
import java.util.HashSet;
public class QuickTest {
public static void main(String[] args) {
HashSet<String> set = new HashSet<String>();
set.add("blah");
set.add("blah2");
for (String s : set) {
set.remove("blah2");
}
}
}
{code}
If you try to remove any value from the set that is actually in the set, whether it is the current element being iterated or not, causes a CME. Trying to remove an element not in the set will not throw any exception. And if the item you are removing is the *only* item in the set, then you also won't get an exception.
> 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_thrift.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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: (was: async_timeout.diff)
> 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_thrift.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.
[jira] Issue Comment Edited: (THRIFT-845) async client does not
respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12900199#action_12900199 ]
Ning Liang edited comment on THRIFT-845 at 8/19/10 3:33 AM:
------------------------------------------------------------
How about:
{code}
// Timeout any existing method calls
private void timeoutIdleMethods() {
Iterator<TAsyncMethodCall> iterator = timeoutWatchSet.iterator();
while (iterator.hasNext()) {
TAsyncMethodCall methodCall = iterator.next();
long clientTimeout = methodCall.getClient().getTimeout();
long timeElapsed = System.currentTimeMillis() - methodCall.getLastTransitionTime();
if (timeElapsed > clientTimeout) {
iterator.remove();
String message = "Operation " + methodCall.getClass() + " timed out after " + timeElapsed + " milliseconds.";
methodCall.onError(new TimeoutException(message));
}
}
}
{code}
was (Author: ning):
How about:
// Timeout any existing method calls
private void timeoutIdleMethods() {
Iterator<TAsyncMethodCall> iterator = timeoutWatchSet.iterator();
while (iterator.hasNext()) {
TAsyncMethodCall methodCall = iterator.next();
long clientTimeout = methodCall.getClient().getTimeout();
long timeElapsed = System.currentTimeMillis() - methodCall.getLastTransitionTime();
if (timeElapsed > clientTimeout) {
iterator.remove();
String message = "Operation " + methodCall.getClass() + " timed out after " + timeElapsed + " milliseconds.";
methodCall.onError(new TimeoutException(message));
}
}
}
> 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_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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12899610#action_12899610 ]
Ning Liang commented on THRIFT-845:
-----------------------------------
Also, yes, we should definitely note the minimum java version for fixing the broken Selector.select() issue (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6525190)
> 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, async_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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: async_timeout_manage_set.diff
I realized I made an oversight - we need to be very careful about how we manage the timeout watch set to avoid a buildup of set members.
> 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, async_timeout.diff, async_timeout_manage_set.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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12901551#action_12901551 ]
Ning Liang commented on THRIFT-845:
-----------------------------------
Hey Eric,
You bring up some good points. We have indirect communication between the client manager and client going through the method call object, which we can refactor into direct communication. I'd like to keep the state machine separate from the async client manager, though - the client manager only coordinates transitioning the method call on ready. Responses inline for the other comments:
1. Timeouts per-transition aren't really what I want as a consumer of this api. It looks like there are about five states, which as a consumer I need to know since the effective timeout on the whole call is then 5x the one I set. I'd rather be guaranteed I will be called back one way or another within the timeout i set, counting time i spend in the selector's queue, etc. Seems like that would be easy to implement.
- The naming is misleading - we can have idle socket timeout or an overall method call timeout (more like a deadline). I coded for the former case, so we can modify to the latter or rename. Modifying is trivial - we just need to track the method call start time, as opposed to the last action time. The effective deadline is arbitrarily long, since we're updating the last action timestamp on each socket read/write, which doesn't necessarily correspond to a transition between method states.
2. You should not be allowed to set a timeout lower than TAsyncClientManager.SELECT_TIME, and the value there should be much lower, maybe 5ms? Because its per-transition as above, the true minimum timeout for an entire call is 1s if each of its states took 200ms. We've actually run clients with lower than 200ms timeouts.
- Agreed, though we won't be able to get to 5ms resolution. The reason is because the JVM makes no guarantees on actual sleep time. Maybe a better solution would be to use Selector.selectNow(), which returns immediately with all ready keys, making our timeout granularity as small as possible.
3. TAsyncMethodCall.onError does not do anything to remove itself from TAsyncClientManager.timeoutWatchSet, so unless it shows up in selectedKeys() it will stay there until its timeout. Not a big deal, but could make that set prohibitively large if there are many errors within a timeout interval.
- TAsyncClientManager removes the method after a transition if the method's client.hasError(). Again, somewhat confusing since it's relying on the method calling the client's onError.
4. We call TAsyncMethodCall.transition before adding it to timeoutWatchSet, so lastTransitionTime is initialized before it is inspected. But, this is a somewhat dangerous pattern to allow an uninitialized primitive to ever be accessible. You should probably initialize it to the current time or a sentinel on instantiation.
- Current time is good - if we end up using the deadline definition of timeout, we don't even have to update the value.
5. it might be safer for lastTransitionTime = System.currentTimeMillis() to be at the top of transition(SelectionKey key) so you can guarantee the timestamp is updated even for errant transitions...don't think it matter's functionally in the current code, but it could in refactorings. you don't want to inadvertently obscure an error with a timeout.
- Good point. I wanted the timestamp to reflect the last socket activity - putting at the beginning of transition could make it inaccurate on long socket reads/writes. Maybe we can use the finally block?
6. TNonblockingServerSocket is the only non-test, non-tutorial thrift source file that uses System.err.println instead of LOGGER.warn
- We can easily fix.
7. You might want to catch Exception instead of Throwable? I don't think you intend or want to catch things like OutOfMemoryError, etc. The docs say "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch"
- Also easily fixed, though I want to make sure we didn't do this for a reason earlier. Bryan, why Throwable vs Exception?
8. TAsyncClientManager.call should really throw an exception if its SelectThread is not running (because its run method did not catch an unexpected Throwable) instead of inserting it into a queue that will grow unbounded and cause an OOM. You probably want a catch and throw block in run that cleans up by calling onError on all pending calls and sets its done flag. You probably also want to do this on ClosedSelectorException instead of just catching and logging it in an infinite loop. It's odd that you catch it on selectedKeys() but not select().
- That catch got folded into the catch(Throwable)
9. timeoutIdleMethods would be a bit faster if it only called currentTimeMillis() once instead of per-check
- Good point. I was going for accuracy but even for large timeout watch sets the iteration time should be negligible.
10. Probably an unnecessary optimization, but instead of relying on Object.hashCode for removing timed out methods, you could identify them by their start timestamp from currentTimeMillis() + a few bits of a static atomic sequence counter, put them in a TreeSet, and shortcut the iteration for timed out methods when you hit the first one that's not too old.
- Definitely, this was the original plan. We'll get it in on next iteration.
> async client does not respect timeout
> -------------------------------------
>
> Key: THRIFT-845
> URL: https://issues.apache.org/jira/browse/THRIFT-845
> Project: Thrift
> Issue Type: Bug
> Components: Java - Library
> Affects Versions: 0.4, 0.5
> Reporter: Eric Jensen
> Assignee: Ning Liang
> Fix For: 0.5
>
> Attachments: async_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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: (was: async_timeout_manage_set.diff)
> 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_thrift.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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12900307#action_12900307 ]
Bryan Duxbury commented on THRIFT-845:
--------------------------------------
That looks right. Is that in the latest rev of the patch?
> 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_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.
[jira] Issue Comment Edited: (THRIFT-845) async client does not
respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12900199#action_12900199 ]
Ning Liang edited comment on THRIFT-845 at 8/19/10 3:33 AM:
------------------------------------------------------------
How about:
{code}
// Timeout any existing method calls
private void timeoutIdleMethods() {
Iterator<TAsyncMethodCall> iterator = timeoutWatchSet.iterator();
while (iterator.hasNext()) {
TAsyncMethodCall methodCall = iterator.next();
long clientTimeout = methodCall.getClient().getTimeout();
long timeElapsed = System.currentTimeMillis() - methodCall.getLastTransitionTime();
if (timeElapsed > clientTimeout) {
iterator.remove();
String message = "Operation " + methodCall.getClass() + " timed out after " + timeElapsed + " milliseconds.";
methodCall.onError(new TimeoutException(message));
}
}
}
{code}
was (Author: ning):
How about:
{code}
// Timeout any existing method calls
private void timeoutIdleMethods() {
Iterator<TAsyncMethodCall> iterator = timeoutWatchSet.iterator();
while (iterator.hasNext()) {
TAsyncMethodCall methodCall = iterator.next();
long clientTimeout = methodCall.getClient().getTimeout();
long timeElapsed = System.currentTimeMillis() - methodCall.getLastTransitionTime();
if (timeElapsed > clientTimeout) {
iterator.remove();
String message = "Operation " + methodCall.getClass() + " timed out after " + timeElapsed + " milliseconds.";
methodCall.onError(new TimeoutException(message));
}
}
}
{code}
> 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_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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Eric Jensen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12901112#action_12901112 ]
Eric Jensen commented on THRIFT-845:
------------------------------------
also, reading through the jetty selector, i noticed they do:
651 if (!key.isValid())
652 {
653 key.cancel();
in their selector loop. we do:
if (!key.isValid()) {
continue;
}
we should probably log at info level or something here...i fear we could be seeing the same invalid keys over and over again and burning more and more cpu skipping them over time. maybe we should be logging the size of Selector.keys(), pendingCalls, and timeoutWatchSet at debug level periodically so we have visibility into leakage errors.
> async client does not respect timeout
> -------------------------------------
>
> Key: THRIFT-845
> URL: https://issues.apache.org/jira/browse/THRIFT-845
> Project: Thrift
> Issue Type: Bug
> Components: Java - Library
> Affects Versions: 0.4, 0.5
> Reporter: Eric Jensen
> Assignee: Ning Liang
> Fix For: 0.5
>
> Attachments: async_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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12901552#action_12901552 ]
Ning Liang commented on THRIFT-845:
-----------------------------------
Also, we do a remove() call on the iterator while looping through methods, which should remove the key from the selector's key set.
> async client does not respect timeout
> -------------------------------------
>
> Key: THRIFT-845
> URL: https://issues.apache.org/jira/browse/THRIFT-845
> Project: Thrift
> Issue Type: Bug
> Components: Java - Library
> Affects Versions: 0.4, 0.5
> Reporter: Eric Jensen
> Assignee: Ning Liang
> Fix For: 0.5
>
> Attachments: async_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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Eric Jensen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12899211#action_12899211 ]
Eric Jensen commented on THRIFT-845:
------------------------------------
Woop woop! Thanks for doing this. Did you look at that jetty selector loop? It has me scared. Hopefully we dont need all that crap if we upgrade the jvm...maybe thrift async should have release notes to that effect?
eric
> 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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12900387#action_12900387 ]
Ning Liang commented on THRIFT-845:
-----------------------------------
Yes, great catch, btw.
> 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_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.
[jira] Closed: (THRIFT-845) async client does not respect timeout
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury closed THRIFT-845.
--------------------------------
Assignee: Ning Liang
Resolution: Fixed
Great, I just committed the latest version (with minor whitespace edits). Thanks for the ambitious patch, Ning!
> 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
> Assignee: Ning Liang
> Fix For: 0.5
>
> Attachments: async_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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: async_thrift.diff
Don't use new Date().getTime(). Use System.currentTimeMillis() instead.
- Done
In timeoutIdleMethods, you shouldn't be calling remove() within the loop. This will cause a ConcurrentModificationException if there is more than one item in the set.
- Only the select thread modifies the timeoutWatchSet - methods are entered when they are popped off the pending queue, not when they're pushed on.
In transitionMethods, you don't need to check if a given method call is in the watch set before trying to remove it - the remove operation will just return false if the item wasn't present in the first place.
- Done
Also fixed a bug with how we do timeout checks.
> 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, async_thrift.diff, async_timeout.diff, async_timeout_manage_set.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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: (was: async_client_timeout.diff)
> 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_thrift.diff, async_timeout_manage_set.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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: async_timeout.diff
How about:
// Timeout any existing method calls
private void timeoutIdleMethods() {
Iterator<TAsyncMethodCall> iterator = timeoutWatchSet.iterator();
while (iterator.hasNext()) {
TAsyncMethodCall methodCall = iterator.next();
long clientTimeout = methodCall.getClient().getTimeout();
long timeElapsed = System.currentTimeMillis() - methodCall.getLastTransitionTime();
if (timeElapsed > clientTimeout) {
iterator.remove();
String message = "Operation " + methodCall.getClass() + " timed out after " + timeElapsed + " milliseconds.";
methodCall.onError(new TimeoutException(message));
}
}
}
> 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_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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: (was: async_thrift.diff)
> 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_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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: async_client_timeout.diff
Attached is a patch that implements a first draft of timeouts. Summary:
1) Allow setting timeout on calling client, through either constructor or a setter
2) On a method call, note the start time if the calling client timeout is > 0
3) On each method transition, update the time stamp
4) Scan through all method calls with timeouts in selector thread, calling onError with a TimeoutException for those that have exceeded idle limit.
Added a test case in TestAsyncClientManager
> 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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12899929#action_12899929 ]
Bryan Duxbury commented on THRIFT-845:
--------------------------------------
Don't use new Date().getTime(). Use System.currentTimeMillis() instead.
In timeoutIdleMethods, you shouldn't be calling remove() within the loop. This will cause a ConcurrentModificationException if there is more than one item in the set.
In transitionMethods, you don't need to check if a given method call is in the watch set before trying to remove it - the remove operation will just return false if the item wasn't present in the first place.
> 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, async_timeout.diff, async_timeout_manage_set.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.
[jira] Updated: (THRIFT-845) async client does not respect timeout
Posted by "Ning Liang (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ning Liang updated THRIFT-845:
------------------------------
Attachment: async_timeout.diff
-Could we move the tracking of last action into TAsyncMethodCall? I'm wary of putting hashmap accesses into such a sensitive loop.
Done. Although I'm keeping a hash set of TAsyncMethodCall to loop over for timing out.
-I'd prefer if you stored long milliseconds since epoch instead of Date objects. We don't need the Date abstraction.
Done.
-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.
Done. Good catch.
> 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, async_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.
[jira] Commented: (THRIFT-845) async client does not respect
timeout
Posted by "Eric Jensen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12901111#action_12901111 ]
Eric Jensen commented on THRIFT-845:
------------------------------------
this is awesome for us...thanks so much for implementing it. something about the way the timeout, selector keys, client state, and method state are spread across several classes makes it scary to me...there's probably a way to refactor into something more centralized that would be easier to reason about. could we make at least one of these things stateless?
for example, when we time something out we call methodCall.onError, which closes the transport, which i think prevents it from showing up in selectedKeys() again, but if it did we would throw our IllegalStateException because we set the error state but didn't cancel the key (since we didn't have access to it from the timeout loop).
also, a few small things, probably not worth re-opening, but worth fixing in another patch:
1. Timeouts per-transition aren't really what I want as a consumer of this api. It looks like there are about five states, which as a consumer I need to know since the effective timeout on the whole call is then 5x the one I set. I'd rather be guaranteed I will be called back one way or another within the timeout i set, counting time i spend in the selector's queue, etc. Seems like that would be easy to implement.
2. You should not be allowed to set a timeout lower than TAsyncClientManager.SELECT_TIME, and the value there should be much lower, maybe 5ms? Because its per-transition as above, the true minimum timeout for an entire call is 1s if each of its states took 200ms. We've actually run clients with lower than 200ms timeouts.
3. TAsyncMethodCall.onError does not do anything to remove itself from TAsyncClientManager.timeoutWatchSet, so unless it shows up in selectedKeys() it will stay there until its timeout. Not a big deal, but could make that set prohibitively large if there are many errors within a timeout interval.
4. We call TAsyncMethodCall.transition before adding it to timeoutWatchSet, so lastTransitionTime is initialized before it is inspected. But, this is a somewhat dangerous pattern to allow an uninitialized primitive to ever be accessible. You should probably initialize it to the current time or a sentinel on instantiation.
5. it might be safer for lastTransitionTime = System.currentTimeMillis() to be at the top of transition(SelectionKey key) so you can guarantee the timestamp is updated even for errant transitions...don't think it matter's functionally in the current code, but it could in refactorings. you don't want to inadvertently obscure an error with a timeout.
6. TNonblockingServerSocket is the only non-test, non-tutorial thrift source file that uses System.err.println instead of LOGGER.warn
7. You might want to catch Exception instead of Throwable? I don't think you intend or want to catch things like OutOfMemoryError, etc. The docs say "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch"
8. TAsyncClientManager.call should really throw an exception if its SelectThread is not running (because its run method did not catch an unexpected Throwable) instead of inserting it into a queue that will grow unbounded and cause an OOM. You probably want a catch and throw block in run that cleans up by calling onError on all pending calls and sets its done flag. You probably also want to do this on ClosedSelectorException instead of just catching and logging it in an infinite loop. It's odd that you catch it on selectedKeys() but not select().
9. timeoutIdleMethods would be a bit faster if it only called currentTimeMillis() once instead of per-check
10. Probably an unnecessary optimization, but instead of relying on Object.hashCode for removing timed out methods, you could identify them by their start timestamp from currentTimeMillis() + a few bits of a static atomic sequence counter, put them in a TreeSet, and shortcut the iteration for timed out methods when you hit the first one that's not too old.
> async client does not respect timeout
> -------------------------------------
>
> Key: THRIFT-845
> URL: https://issues.apache.org/jira/browse/THRIFT-845
> Project: Thrift
> Issue Type: Bug
> Components: Java - Library
> Affects Versions: 0.4, 0.5
> Reporter: Eric Jensen
> Assignee: Ning Liang
> Fix For: 0.5
>
> Attachments: async_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.