=rem #18339 Use explicit handshake timeout #18542
Conversation
|
Refs #18339 |
|
This should be merged for 2.4.1, it is probably only a problem when using test transport. |
| case Event(HandshakeTimer, InboundUnassociated(_, wrappedHandle)) ⇒ | ||
| sendDisassociate(wrappedHandle, Unknown) | ||
| stop(FSM.Failure(TimeoutReason("No response from remote for inbound association. Handshake timed out after " + | ||
| s"[${settings.HandshakeTimeout.toMillis} ms]."))) |
There was a problem hiding this comment.
I'm unsure about the timeout for inbound associations. It was not handled previously, and is perhaps not needed, but it might not hurt either?
There was a problem hiding this comment.
I think it should have this timeout.
|
|
||
| # The timeout for outbound associations to perform the handshake. | ||
| # If the transport is akka.remote.netty.tcp or akka.remote.netty.ssl | ||
| # the configured connection-timeout for the transport will be used instead. |
There was a problem hiding this comment.
I am not sure about that. The connection-timeout relates to TCP events, which is handled by the kernel, while the Akka Protocol is at the JVM level. I am not sure these timeouts mix well.
There was a problem hiding this comment.
I remember a customer issue in which SSL was taking so long to init that connections timed out. So from that perspective, if I read this PR correctly it would be easier to tune for their case as well.
The error message would also have been nicer, so I'm for this PR, though maybe I'm missing some more fuzzy interplay with other remoting things
There was a problem hiding this comment.
I was thinking that otherwise the user would always have to change both those settings. When would you use different timeout here than the connection-timeout?
There was a problem hiding this comment.
on a busy system the actual TCP connection might finish much quicker than a remoting handshake, but maybe I am just too paranoid. Less knobs to tune is maybe better.
There was a problem hiding this comment.
but isn't that same issue from the user's perspective? at least all sane users.
There is also a backwards compatibility aspect of this.
Let's say that someone has configured connection-timeout = 30s, then we can't suddenly introduce a new property that fail the connection attempt after 15s (which is the default for handshake-timeout)
There was a problem hiding this comment.
Both are good points, convinced :)
|
Can you add the two timeout test cases to AkkaProtocolSpec? That tests this actor in isolation. |
|
Test FAILed. |
c19651c to
8418b14
Compare
|
Test FAILed. |
|
PLS BUILD |
|
@drewhk please take a look at the latest here so this can be merged |
|
This PR received the Seal of approval from the Remoting Department. |
|
Test FAILed. |
|
thanks, ah bin compatibility issue |
* instead of using transport failure detector * add a new config property akka.remote.handshake-timeout, but for netty.tcp and netty.ssl the existing netty.tcp.connection-timeout setting will be used * add test of the timeouts * mima filter for internal ProtocolStateActor
8418b14 to
94896e8
Compare
|
Test PASSed. |
=rem #18339 Use explicit handshake timeout
for netty.tcp and netty.ssl the existing netty.tcp.connection-timeout
setting will be used