Ensure Connection and Channel cancellation token properly float into handlers

[danielmarbach]

I started doing this and then soon realized the work is probably more involved. In general, it would likely be good to figure out what cancellation behavior we want as part of close and shutdown and then with that agreement go through the internal APIs again that are getting the AsyncEventArgs.

There are many more cases like the one in the PR that need to be properly thought through

Fixes #1787

[paulomorgado]

@danielmarbach, do you have any scenario in mind for the use of the cancellation token in the event args?

[danielmarbach]

@danielmarbach, do you have any scenario in mind for the use of the cancellation token in the event args?

Not sure I understand your question. Can you elaborate? The token is part of the event args. The entire idea is that cancelation can be observed in the handlers as well

[paulomorgado]

@danielmarbach, do you have any scenario in mind for the use of the cancellation token in the event args?

Not sure I understand your question. Can you elaborate? The token is part of the event args. The entire idea is that cancelation can be observed in the handlers as well

I understand that. I was just wondering in what scenario I would want to observe that cancelation token and what could I do with it.

[danielmarbach]

@danielmarbach, do you have any scenario in mind for the use of the cancellation token in the event args?

Not sure I understand your question. Can you elaborate? The token is part of the event args. The entire idea is that cancelation can be observed in the handlers as well

I understand that. I was just wondering in what scenario I would want to observe that cancelation token and what could I do with it.

Got it. The intent is that any handler method as the chance to participate in cooperative cancellation and given event handler delegates to not follow the regular method parameter guidance the cancellation token is part of the event args.

[lukebakken]

We need to ensure that this PR fixes #1787

[danielmarbach]

@lukebakken I briefly looked through this code again and I think overall what currently is confusing is the number of cancellation tokens we have. For example CloseAsync has an overload that accepts ShutdownEventArgs that contains a cancellation token but the method itself also supports a token.

If we can cleanup things a bit so that it is always clear what token should be passed around, it might be possible to have a cancellation token managed by the dispatcher that then gets passed to the work struct which is then passed down to the handle methods and passed into the corresponding event args.

Would WaitForShutdownAsync be an appropriate place?

[danielmarbach]

@lukebakken I pushed three new commits that show a direction as a draft idea. I'm running out of time today, but there is a lot more to cover, I guess.

There are probably quite a few things we need to discuss around the token triggering. Currently it immediately triggers when quiesce but maybe we want to have some configurable grace period? Another other option could be that we never trigger the token unless the token is triggered that is passed to Close.

The channel and sessions events are not wired up properly either.

[lukebakken]

Thanks @danielmarbach - I'll return to this within a couple days, hopefully!

[danielmarbach]

Something doesn't fit quite right. I have been spending a bit more time spelunking around. I haven't fully finished my investigations, but at first sight it looks like it might be beneficial to change the API surface of the methods that accept the ShutdownEventArgs to no longer accept cancellation tokens. This might simplify the code base quite a bit.

[danielmarbach]

I pushed two commits that go in that direction. Probably need to think through this more and also discuss whether there is a good backward compatible way to ship this

[michaelklishin]

@danielmarbach my apologies about a complication to the contribution process but before the RabbitMQ core team can accept this contribution, we need you to sign Broadcom's contributor CLA.

It's done online using Box Sign and the CLA document is just over one page long.

Note that the signing party can be your employer, in which case the CLA is only required to be signed once, not for every contributor.

[danielmarbach]

@michaelklishin ok I submitted the request

[danielmarbach]

I have signed the new CLA. I will come back to this once I have some free time

[danielmarbach]

What should the expected behavior for AsyncEventingWrapper be? By wiring up more tokens, the async actions become cancellable and therefore throw OperationCancelledException. As of today, the wrapper does a general catch and when the exception handler is set, invokes that one. Is that desirable, or should it not invoke the exception handler for OperationCancelledException since they are expected to happen?

[lukebakken]

should it not invoke the exception handler for OperationCancelledException since they are expected to happen?

I've been so wrapped up in debugging this that I had to re-read you paragraph a couple times to get my brain back into "C# mode" 😸

You're correct, it doesn't make sense to call the exception handler for anything related to cancellation.

[danielmarbach]

@lukebakken I'm honestly not sure if I understand the ins and outs of the library well enough to be able to take this to completion. Any help would be appreciated in case we want to pull this in.

[lukebakken]

@danielmarbach I appreciate all of the effort you've put into this PR! I got the OK to spend some time on this library so I'll be doing so, starting tomorrow. Stay tuned.

[lukebakken]

@danielmarbach hello! I have spent some time on this PR. I restored a couple methods that would have broken backwards compatibility. On first review, this changes should fix issue #1787 for the original issue as well as for this comment.

I'm going to look into this comment that you made but otherwise, I don't think anything else needs to be done at this time.

[danielmarbach]

@lukebakken I mostly ran into issues with adding more tests because some of the invocations of the event handlers run "inline" which then makes it possible to trigger further actions to make the test not deadlock so I gave up

[danielmarbach]

Thanks for taking this to completion. I was too swamped

[lukebakken]

Hey thanks for getting the ball rolling!