MessageRequest callbacks never triggered if disconnected

[jshslt]

Hi,
We've found that the curl multi interface with easy handles that HTTP2Transport uses is very "durable" - that is, disconnection of the underlying sockets don't appear to be readily raised to the SDK's level.

example -
with an active AVS session, disconnecting the network interface does not raise to a connection status change, at the SDK level. the connection is quietly restored when the network interface is restored.

We find this to be a problem, in particular, when sending events through MessageRouter::send - if the underlying transport is disconnected, we never get onSendCompleted/onExceptionReceived callbacks for the MessageRequest object.

furthermore - If the connection is restored, MessageRequests that were waiting on the old transport are lost (without callback).

Our suggestion is that HTTP2Transport::networkLoop might need some more explicit checking of connection status, as well as timeouts for outgoing messages.

Thanks

[scotthea-amazon]

Hello jshlt,
Thank you for bringing this to our attention! We will investigate and get back to you.
-SWH

[scotthea-amazon]

Hello again jshlt,
I have NOT verified the problems that you are seeing, but there are a couple of known issues in MessageRouter::send() that may be the same or closely related:

1. The lambda function is not acquiring m_connectionMutex before using m_activeTransport. see Issue 5
2. The lambda function is missing an else clause to signal onSendCompleted(NOT_CONNECTED)
   We are preparing a fix to MessageRouter.cpp for both in the next update, along the following lines:

void MessageRouter::send(std::shared_ptr<avsCommon::avs::MessageRequest> request) {
    auto task = [this, request]() {
        std::shared_ptr<TransportInterface> transport;
        {
            std::lock_guard<std::mutex> lock{m_connectionMutex};
            transport = m_activeTransport;
        }
        if (transport && transport->isConnected()) {
            transport->send(request);
        } else {
            request->onSendCompleted(avsCommon::avs::MessageRequest::Status::NOT_CONNECTED);
        }
    };
    m_sendExecutor->submit(task);
}

Please let us know if this resolves the issue for you,
-SWH

[scotthea-amazon]

Hello jshslt,

Sadly, I have to report that I am able to reproduce the problem you describe both with and without the change I suggested above. Digging in to this it looks like there may be multiple problems.

One of these is that AVSConnectionManager::send() / MessageRequest level there is no interface for specifying a timeout for sending a message. Since AVSConnectionManager handles reconnecting to AVS internally, MessageRequests will only very rarely receive onSendCompleted() with the NOT_CONNECTED status. Instead they are left in limbo indefinitely, as you have observed.

We are actively working on this and will let you know when we have a fix.

Thank you again for bringing this to our attention!
-SWH

[jshslt]

@scotthea-amazon - thanks - just wanted to check in and see if you might know an ETA on a fix for this?

[scotthea-amazon]

@jshlt - No ETA at the moment. We will let you know as soon as we have something available.

[scotthea-amazon]

Hello jshlt,

Changes were pushed in the 0.5 release that address most of the points you raised in this issue. The primary change are:

• When a disconnect is detected, onSendCompleted() notifications will be sent to all outstanding MessageRequests.
• When the request/reply of a message does not progress, onSendCompeted() will be called with a TIMEDOUT status.

An outstanding issue is that detection of the disconnected state can be quite slow. libcurl does not immediately detect an unconnected network cable and a request will not fail until the operation has timed out. This is discussed in more detail on the libcurl website here.

Please let us know how this update works for you,
-SWH

[mradulan]

Closing the issue for now. Please re-open if the problem still exists.