HttpClient _timeout and _soTimeout is messed up

Attached a patch for fix this issue.

Can you check this

JETTY-1335 is somewhat related to this JIRA issue as it talks about timeouts on connect in the HttpClient's SelectConnector..

I am not sure I understand the report.

The timeout queue is ticked every 200 ms, so setting a timeout of 100 ms is useless, since the granularity is at least 200 ms.

Plus, I doubt that people wants to setup a 100 ms timeout for read/connect, considering that a normal young generation collection may take about that time, so the application is stopped while the GC is running, so all the connections will timeout.

There has been a recent refactoring following https://bugs.eclipse.org/bugs/show_bug.cgi?id=337678 which I think solves the connect problems, but I do not understand if this bug reports refers to read timeouts or not.

Can you please expand on what is the use case that is not working for you ?
If the concern is about being able to set the timeout to 100 ms, I am guessing it's not a valid use case; I'm interested in further details if I am wrong on the use case validity.

I am double checking the semantic of HttpExchange.timeout, but will eventually open another bug related to that only if it's not correct.

Thanks !

Hi Simone,

The issue is that, as HttpClient._timeout is used as soTimeout, socket read timeout is always triggered before the main timeout (the HttpClient_timeout + HttpExchange.timeout /delay/ one). In other words HttpExchange.timeout is completely ignored. There's no longer such a feature as timeout per request.

In our usecase we need to have full control over timeout value. Timeout values are set on per request basis and the range of possible values is really wide. That is why, we're using a tiny value for HttpClient_timeout. We just use a positive value that will not add a large bias to per request timeout values. It is HttpExchange.timeout that really specifies the timeout. While I know that this HttpExchange.timeout is semantically a delay, so using small HttpClient_timeout value and huge HttpExchange.timeout value seems weird. However, technically it works the way we need it to work .

Thanks,
Bartek

Bartek, you found a workaround to a bug, but in reality HttpExchange.timeout should override HttpClient.timeout, not add to it like it does now (which is a bug, we did not intend that semantic, but rather the override semantic).

Note also that the SO timeout is without meaning for non-blocking channels (it only works for blocking channels) so we have a different mechanism for timing out non-blocking channels.

If HttpExchange.timeout overrides HttpClient.timeout properly, would that be enough to cover your use case ?
In that way, for each of your request you set the timeout on the exchange, and have a default timeout set on the client for exchanges where the timeout is not explicitly set.

I am working on this right now, because it's a clear bug that HttpExchange.timeout does not override HttpClient.timeout.
Let us know if this fix would work for you.

See https://bugs.eclipse.org/bugs/show_bug.cgi?id=337896 for the HttpExchange.timeout bug.

Current HttpClient _timeout is used as the duration timeout in the HttpClient like this

    @Override
    protected void doStart() throws Exception
    {
        super.doStart();

        _timeoutQ.setDuration(_timeout);
        _timeoutQ.setNow();
        _idleTimeoutQ.setDuration(_idleTimeout);
        _idleTimeoutQ.setNow();
...

So we can't treat it as the timeout value for socket read timeout.
When set the HttpClient._timeout to be 100ms, it just let _timeoutQ check the tasks more often.

But if you treat the HttpClient._timeout as the HttpClient._soTimeout and HttpClient._timeout as the default value of Exchange._timeout, I don't think it is right also.
Eg, the client is requesting a large amount response, it can alway read something from the socket which will not trigger the socket timeout , but the whole HttpClient processing time could exceed the HttpClient._timeout.

BTW, I'm very interesting about this "Note also that the SO timeout is without meaning for non-blocking channels (it only works for blocking channels) so we have a different mechanism for timing out non-blocking channels." Can you show me the code in the Jetty HttpClient? I just want to make sure we understand the timeout value rightly.

Hi Simone, Willem,

First of all, timeouts per request were introduced by my patch contributed to JETTY-912 (Jetty 6). Somebody also ported these changes to Jetty 7. I called the value attached to the exchange 'delay' and not 'timeout'. I wanted these values to be mapped directly to the interface that the Jetty's Timeout class exposed and didn't like the idea of being forced to refactor the Timeout class itself . This HttpExchange's 'delay' was later renamed to 'timeout' while technically it was still a delay. It wasn't my intention to call it that way - to call it 'timeout' .

The approach that you implemented (HttpExchange.timeout overrides HttpClient.timeout) is definitely cleaner. It's more intuitive and covers my usecase. However, I'm concerned with the complexity it introduced to Jetty client's code. Now, when a task is scheduled, HttpClient.timeout is first subtracted from HttpExchange.timeout and later added to the new value inside the Timeout class, right? . I'm also wondering how is a situation when HttpClient.timeout>HttpExchange.timeout handled. I haven't done a full code review, as I don't have Jetty 7 trunk checked out, but I'll need to check how Jetty 7 behaves in such a scenario.

SO timeout & non-blocking channels:
I'm not a NIO guru, but I agree that if a socket channel is running in a non-blocking mode, so_timeout should not be used at all. That's intuitive . Only non-blocking operation (like select()) should be used, so timeout should have no effect.
However, with the old (Jetty 7.2.2) code:

 SocketChannel channel = SocketChannel.open();
            Address address = destination.isProxied() ? destination.getProxy() :  destination.getAddress();
            channel.configureBlocking( true );
            channel.socket().setTcpNoDelay(true);
            channel.socket().setSoTimeout(_httpClient.getConnectTimeout());
            channel.connect(address.toSocketAddress());
            channel.configureBlocking(false);
            channel.socket().setSoTimeout((int)_httpClient.getTimeout());

            _selectorManager.register( channel, destination ); 

we observed weird behavior. It looked as if the client (the select connector) was closing the connection and server was unable to return response. This happened after a really short time as HttpClient.timeout was small and it was used as soTimeout. At the same time, after client changed the state of the socket so that server was unable to return response, HttpClient did not let our client know that something wrong happened. None of te HttpExchange callback methods was called. As channel.socket().setSoTimeout() is no longer used in SelectConnector, it's not really a problem. However, I'd be nice to understand what was going on as there's still a default value for soTimeout. If it's used, it might cause some issues in the future.
Maybe changing the mode from blocking to non-blocking introduces some problems? Just a guess.

Best regards,
Bartek

@Willem: again, there is no value to set the HttpClient.timeout to 100 ms, it's not how Jetty's Timeout work (and it was a workaround to another bug).
If you have cases where long responses could last more than the timeout, then the solution is to increase the timeout to avoid that the connection is closed in the middle of the response.
Regarding the timeout for non-blocking channels, there is nothing different to look at. We use the same mechanism for both blocking and non-blocking channels, but it's not setting SO timeout (that's what I meant when I said that we have a different mechanism) - it's based on Jetty's Timeout class.

@Bartek: if HttpClient.timeout > HttpExchange.timeout then the "delay" is negative but the exchange it will expire at the correct time. I have added a test case for this scenario as well.
The code you reported is for the connect phase, and is changed in current trunk where we no longer call setSoTimeout() if the channel is non-blocking.
We spotted another problem regarding the interaction with HttpClient.timeout, reported here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=337898

The whole point is that HttpExchange.timeout had the wrong behavior due to a bug.
With the behavior fixed, no need to set any timeout to small values, everything works more intuitively and your use cases are supported.

If you're happy, I'll mark this issue as resolved.

Simone,

The 337898 bug seems to explain what was happening in our Jetty 7.2.2 scenarios. It seems that all of our problems will vanish magically when we switch to the next version of Jetty. I'm ok with closing this issue.

When will these fixes be made available in a released version of Jetty? What exact version will it be?

Thanks,
Bartek

Jetty 7.3.1 will incorporate these changes, and will be available in around 10 or so days.

Fixed in 7.3.1.