amqp / rhea-promise

A promisified layer over rhea AMQP client
Apache License 2.0
30 stars 19 forks source link

[bugfix] improves behavior of session/receiver/sender creation methods during cancellation #83

Closed chradek closed 3 years ago

chradek commented 3 years ago

Description

When createSession, createReceiver, or createSender are cancelled via an abortSignal, the underlying rhea analogue is closed, regardless of if it has actually been opened.

In the @azure/event-hubs library, I ran into an edge case caused by this behavior. In @azure/event-hubs, I had a scenario where I'd create multiple receivers expecting an error from the service, and then triggered the abortSignal as soon as the 1st receiver was created. Any receivers that weren't opened prior to the abortSignal being triggered would cause the service to respond with an error stating they could not find the receiver's session handle in the connection.

This error from the service makes sense because from the service's point of view, it never saw a session for the receiver created.

This PR adds a check before closing the underlying rhea resource that ensures it is actually open 1st.

Edit:

I updated the logic around when to close the rhea session/receiver/sender when cancelling their creation.

Through some more testing I found that each resource's open event could still be emitted after aborting even if resource.is_open() returned false. As pointed out in the comments, is_open() returns false if the local is open but the remote is not. So what's happening is that the begin/attach frame is send to the peer, we abort, then the peer responds saying it began/attached.

So now I check if the resource is already closed. If it isn't, I attach a listener for the resource's open event and call close on that resource if the event is triggered. This ensures that the resource is removed from its parent's state.

I've updated the tests to account for this and ensure that the resources are properly removed from their parent. I also found that in the "pre-cancelled" cases, we were creating the resources and the aborting, rather than just aborting right away, so I've updated this as well.