AsyncRpcContinuation should be disposed

[danielmarbach]

Proposed Changes

I spent a few minutes looking through the discussion #1721 that @phatboyg raised and saw that even though the RPC continuations are always creating cancellation token sources and linked tokens in the constructor we are not always disposing them which can lead to leaks. It is safer to always dispose and also simplifies the code a bit. There were some comments / notes about not disposing deliberately but I couldn't figure out why that should be the case, so I assumed it is a mistake and introduced disposing.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bug fix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

Further Comments

If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.

[danielmarbach]

I have some more thoughts about the code but need to wrap up things for today and they can be handled seperately.

[danielmarbach]

Build fails due to the vulnerability problems mentioned in #1735 and and old version of json.

[lukebakken]

Alright, based on the latest integration test failures, my comment was applicable. We pass the AsyncRpcContinuation's cancellation token into methods like HandleBasicConsumeOkAsync in the consumer dispatcher when the response comes back from RabbitMQ, and by then, the token has already been disposed. The reason to use the same token is that it represents the continuation timeout value, which must extend into the phase where the RPC response is handled.

This is why I checked to see if the command was enqueued or not. If enqueued, the handling of the response would (should) take care of disposal, if the RPC command was not enqueued, then dispose the command object.

Between checking if the RPC object is enqueued, and this code, instances of AsyncRpcContinuation should always be disposed.

Anyways, as is, I think the code works as it should. Let me know what you think. Going to close this PR for now but if there's a way to simplify the code while retaining this behavior that would be great.

[danielmarbach]

Yeah I missed that part. I think if we would put together the creation of the continuation with the acquiring of the semaphore and the enqueuing it would be possible to return a disposable struct that releases the continuation and the RP semaphore or just the RPC semaphore that would reduce nesting and flow logic. I can make a PR to demonstrate it and then we'll see whether it's worth it or not

[danielmarbach]

@lukebakken see #1743

Not sure if it is worth it. It's a bit of a machinery but would make the scope owning explicit and reduce nesting. It would also centralize the decision making more or less into one place

That being said I'm not sure overall that it reduces the cognitive overhead