Remove "-t" enforcement in C++, simplify ConnectTimeout / CloseTimeout #2059
Conversation
| @@ -610,38 +610,6 @@ allTests(Test::TestHelper* helper, const CommunicatorObserverIPtr& obsv) | |||
|
|
|||
| metrics->ice_getConnection()->close(Ice::ConnectionClose::GracefullyWithWait); | |||
|
|
|||
| metrics->ice_timeout(500)->ice_ping(); | |||
There was a problem hiding this comment.
It would be good to add back a metrics test that tests connection failures. It's not clear to me how to do it. I'll create an issue .
There was a problem hiding this comment.
It's actually not totally clear to me why this test is no longer working.
cpp/src/Ice/ConnectionI.cpp
Outdated
| @@ -2685,10 +2702,14 @@ Ice::ConnectionI::sendResponse(OutgoingResponse response, uint8_t compress) | |||
| bool | |||
| Ice::ConnectionI::initialize(SocketOperation operation) | |||
| { | |||
| if (_connectTimeout > chrono::seconds::zero()) | |||
There was a problem hiding this comment.
Isn't it possible for initialize to be called multiple times?
There was a problem hiding this comment.
I think it's possible but so rare that we don't need to optimize for the case where it's called multiple times.
If it's called multiple times, we schedule multiple connect timer tasks.
There was a problem hiding this comment.
But isn't it an error to schedule a timeout, which is already scheduled?
There was a problem hiding this comment.
To be clear if we return false from
if (s != SocketOperationNone)
{
_threadPool->update(shared_from_this(), operation, s);
return false;
}
This method would be called again, and the timer schedule would throw
if (!inserted)
{
throw std::invalid_argument("task is already scheduled");
}
There was a problem hiding this comment.
That's not the case because we schedule a different timer task each time. It's created by make_shared:
_timer->schedule(make_shared<ConnectTimerTask>(shared_from_this()), _connectTimeout);
| @@ -2559,6 +2550,11 @@ Ice::ConnectionI::initiateShutdown() | |||
| os.write(static_cast<uint8_t>(1)); // compression status: compression supported but not used. | |||
| os.write(headerSize); // Message size. | |||
|
|
|||
| if (_closeTimeout > chrono::seconds::zero()) | |||
There was a problem hiding this comment.
We will need to document the change of behavior, for both connect and close timeouts.
Previously, the close timeout didn't apply to the whole period of the connection closure. It applied to the period between two transport operations. So as long as there was transport activity while in the closing state, the connection wouldn't timeout. Ditto for connection establishment.
There was a problem hiding this comment.
Yes. Keep in mind the properties are new (compared to Ice 3.7) and the semantics match IceRPC's semantics for connect and shutdown timeout.
cpp/src/Ice/ConnectionI.cpp
Outdated
| @@ -2685,10 +2702,14 @@ Ice::ConnectionI::sendResponse(OutgoingResponse response, uint8_t compress) | |||
| bool | |||
| Ice::ConnectionI::initialize(SocketOperation operation) | |||
| { | |||
| if (_connectTimeout > chrono::seconds::zero()) | |||
There was a problem hiding this comment.
I don't think this is the right place for scheduling the connect timer task. It should be moved to the implementation of startAsync only if the connection establishment doesn't complete right away.
There was a problem hiding this comment.
Moved to startAsync as suggested.
| @@ -610,38 +610,6 @@ allTests(Test::TestHelper* helper, const CommunicatorObserverIPtr& obsv) | |||
|
|
|||
| metrics->ice_getConnection()->close(Ice::ConnectionClose::GracefullyWithWait); | |||
|
|
|||
| metrics->ice_timeout(500)->ice_ping(); | |||
There was a problem hiding this comment.
It's actually not totally clear to me why this test is no longer working.
| controller->holdAdapter(-1); | ||
| try | ||
| { | ||
| to->sendData(seq); |
There was a problem hiding this comment.
Do we have a similar test for the new idle timeout? I suppose that if the connection writes are stuck in both directions the connection is not killed?
There was a problem hiding this comment.
That's correct. With this update, a write can get stuck forever. We actually have exactly the same semantics with IceRPC: with IceRPC, the only way to interrupt a stuck write with the ice protocol is to abort the connection.
The assumption here (and in IceRPC) is if the connection is sick and writes are stuck, the idle check will abort the connection (locally or in the peer). We should indeed add a test to verify the idle check can indeed abort a connection when this connection is stuck writing.
There was a problem hiding this comment.
The removed portion of the metrics test (as I understand it) was generating connection errors by having write hang because of a low "-t" (500 ms). Since this feature (-t) is removed by this PR, this no longer works.
|
@bentoi, can you please review the work-around I added to the Ice/metrics test? |
This PR removes the "-t" (connection timeout) enforcement in C++. It also simplifies the implementation for ConnectTimeout and CloseTimeout, and removes the special deprecated "-2" invocation timeout.