Multiple case about ExchangeContext leaks

[kghost]

Problem

• When OnSessionReleased is called while the exchange is in state of WillSendMessage, the OnExchangeClosing is triggered, but the ref count is not decreased. Will cause that exchange leak.
• In ExchangeContext::HandleResponseTimeout if the application doesn't close the exchange in OnResponseTimeout callback. it leaks.
• In ExchangeContext::SendMessage, if mResponseTimeout is set to 0, it leaks.

Proposed Solution

To prevent possible problem like this one, and make exchange context more robust

• Implement a state machine to manage ref counters
• Make Close/Abort re-entrant

[bzbarsky-apple]

When OnSessionReleased is called while the exchange is in state of WillSendMessage, the OnExchangeClosing is triggered, but the ref count is not decreased. Will cause that exchange leak.

If he consumer is waiting to send on the exchange, the consumer has to release that ref. The exchange cannot release it, because that will leave a dangling pointer.

In ExchangeContext::HandleResponseTimeout if the application doesn't close the exchange in OnResponseTimeout callback. it leaks.

This is not true. The exchange closes itself after calling OnResponseTimeout (by calling MessageHandled()), unless the callee takes action to keep it open, and this is documented in the delegate API.

In ExchangeContext::SendMessage, if mResponseTimeout is set to 0, it leaks.

In what sense? What leaks, exactly? If mResponseTimeout is 0, that means "stay open indefinitely waiting for a response" and the responsibility for closing the exchange is then on the consumer.

Implement a state machine to manage ref counters

This is generally a good idea, but we should not be taking that on for 1.0 at this point.

Make Close/Abort re-entrant

Not sure we should do that, but worth thinking about whether that makes things more or less complex.

[bzbarsky-apple]

And in particular: if the exchange is waiting for a response, the refcount is held by the network stack. The exchange can release that refcount itself. If the exchange is waiting for a send, the refcount is held by the consumer that will do the send. The exchange has no way to release that refcount itself without leaving dangling pointers.

[bzbarsky-apple]

Created #19383 to handle the clarity issue.

[kghost]

If mResponseTimeout is 0, that means "stay open indefinitely waiting for a response" and the responsibility for closing the exchange is then on the consumer.

if the exchange is waiting for a response, the refcount is held by the network stack.

These 2 statement just conflict.

[bzbarsky-apple]

OK, good point. If the exchange is waiting for a response and a response timeout was requested, the network stack will handle closing the exchange on response timeout, unless the consumer takes action to prevent it. The consumer will be notified when the timeout happens, so it can drop its ref to the exchange.

If the exchange is waiting on a response with no timeout allowed, the consumer needs to handle closing the exchange and dropping the reference to it, generally. In the "session released" case we treat things as a timeout, looks like.

I agree this is all a bit more complicated than one would prefer.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.