Ensure connection is closed immediately upon socket timeout

[aldexis]

Problem Description

#508 made a change to HttpRequestExecutor to ensure a prompt connection close, especially upon a SocketTimeoutException, by closing the connection with CloseMode.IMMEDIATE.

However, this does not work properly if the connection's socket was bound not through the bind(SSLSocket, Socket) method added in #468 and instead by passing the SSLSocket directly to bind(SSLSocket).

This is for instance what happens in httpcomponents-client < 5.4 in DefaultHttpClientConnectionOperator.

Proposed Solution

While it's probably undesirable to do what happens in the described example above, this very much can happen. On the other hand, setting the socket timeout to something very low when we want to close it immediately seems reasonable, hence my proposal for this change.

Testing considerations

I am unfortunately not terribly familiar with this codebase and I have done my best to provide a reproducer test, but that had to rely on not using the provided HttpRequester from the test setup, because it is not subject itself to the issue I've described here.
The classes I've used are however all part of the public API, hence seemed reasonable to test directly.

I'd be happy to update or move the test in any way that you would prefer.

[ok2c]

@aldexis Could you please take a look at 830b0620 and let me know if this resolves the issue for you? I would really hate having to use 1 ms timeout in BHttpConnectionBase indiscriminately. If absolutely needed we could provide pluggable DisposalCallback in the public APIs.

[ok2c]

@aldexis Another possibility is to catch SocketTimeoutException and set socket timeout to 1 ms prior to disposal of the connection.

[aldexis]

@ok2c I like the idea of catching SocketTimeoutException specifically to handle this case, which I imagine we could do in HttpRequestExecutor. Ill push that change tomorrow (I'm based in the EU)

With that said, could you explain to me why it would be a bad thing to set such a timeout? The fact that we are requesting an immediate close makes me think that forcing a reduction of the socket timeout would be a desirable property, but Im very willing to believe there are things I'm not aware of.

[ok2c]

With that said, could you explain to me why it would be a bad thing to set such a timeout? The fact that we are requesting an immediate close makes me think that forcing a reduction of the socket timeout would be a desirable property, but Im very willing to believe there are things I'm not aware of.

@aldexis HttpClient is being used by different people in different contexts and "immediate" may mean different things to different people. Generally we cannot make everyone happy and usually tend to move context specific decisions to callbacks or strategy interfaces.

[aldexis]

@ok2c I've made a different change in 00c68e1 but this actually seems weird to me. We're already saying "I want to close this connection immediately" and the fact that we need to set the socket timeout to 1ms in certain condition just seems like an abstraction leak 🤔

To be clear in case it wasn't by the way, in my specific case, I believe simply upgrading http-client past 5.3 will be fine, but it seemed incorrect that requesting a connection to close immediately could hand for potentially a long time

[aldexis]

"immediate" may mean different things to different people

That's a fair point (though I'd be curious to learn about other interpretations and usecases!)

[ok2c]

"immediate" may mean different things to different people

That's a fair point (though I'd be curious to learn about other interpretations and usecases!)

@aldexis Some people may argue 1 ms is too extreme even for the immediate mode. I do not want to find myself having that conversation.

[aldexis]

"immediate" may mean different things to different people

That's a fair point (though I'd be curious to learn about other interpretations and usecases!)

@aldexis Some people may argue 1 ms is too extreme even for the immediate mode. I do not want to find myself having that conversation.

That makes sense. In that case, do you think the current approach would be reasonable?

[ok2c]

@aldexis Cherry-picked to 5.3.x

[ok2c]

That makes sense. In that case, do you think the current approach would be reasonable?

@aldexis I think it is the least intrusive one. And for persistent connection pools there is DisposalCallback

[aldexis]

Thanks for merging this!