Reduce close / abort methods in public API

[bollhals]

Proposed Changes

There are 3 close / abort functions in the public API. This forces a lot of similar implementations in AutorecoveryConnection.
This PR removes all of them except one in the public API and replaces it with extension methods with the same parameters. (This means a recompile should be enough for customers, if they haven't mocked the interface itself)

Types of Changes

• 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

• 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

[bollhals]

Hmm it works with SDK V 5.0, but build still uses 3.1.x which somehow doesn't like the this.Abort() where Abort is an extension method 🤔
Shall I change it to call the Close method directly, or can we update the SDK?

=> not needed anymore, see comment from @danielmarbach

[michaelklishin]

There are some known users that mock the connection API interface. I doubt it's a widely used approach but some prominent projects could have adopted it.
Reducing the minimum required API surface here sounds reasonable to me.

@stebet @bording @danielmarbach @lukebakken WDYT?

[michaelklishin]

Hmm it works with SDK V 5.0, but build still uses 3.1.x which somehow doesn't like the this.Abort() where Abort is an extension method 🤔
Shall I change it to call the Close method directly, or can we update the SDK?

Are there any downsides besides all contributors having to upgrade? I'd say requiring .NET Core 5 for our own development is fine.

[bording]

Reducing the minimum required API surface here sounds reasonable to me.

I'm all for making the Public API simpler, though a quick look at the PR, I'm not quite sure how this PR does that. It seems to just be shuffling the APIs around, not actually reducing anything.

Are there any downsides besides all contributors having to upgrade? I'd say requiring .NET Core 5 for our own development is fine.

It should be perfectly fine to move to a newer SDK because that has no impact on the consumers of the client.

However, something seems off here. I can't think of a reason why the SDK version would impact whether or not the code compiles.

[michaelklishin]

It reduces the part that is mandatory for implementing. We can consider removing the Abort overload that provide access to the reply code and message. I mean, if you are wiling to force close and ignore all errors, that context is likely not very relevant anyway.

[bollhals]

Reducing the minimum required API surface here sounds reasonable to me.

I'm all for making the Public API simpler, though a quick look at the PR, I'm not quite sure how this PR does that. It seems to just be shuffling the APIs around, not actually reducing anything.

The reduction happens at the IConnection and IModel interface. For convenience and to reduce the update effort the existing api are now extension methods.

=> interface smaller, easier to implement it while keeping the public facing api usage the same.

Our benefits are the reduced number of implementation in e.g Connection & AutorecoveringConnection while for the customer nothing changes (except he may calls close now through an extension method)

Are there any downsides besides all contributors having to upgrade? I'd say requiring .NET Core 5 for our own development is fine.

It should be perfectly fine to move to a newer SDK because that has no impact on the consumers of the client.

However, something seems off here. I can't think of a reason why the SDK version would impact whether or not the code compiles.

It is indeed strange, but I have had it happened to me once or twice that they fixed some bug in a newer SDK. This feels just like it.

You can look at the code in question. It's a instance method of e.g. Connection, that calls this.Abort. (This implementing an IConnection and Abort being an extension method on IConnection) So this should be fine, but somehow the old SDK doesn't like it. 🤷‍♂

[bollhals]

It reduces the part that is mandatory for implementing. We can consider removing the Abort overload that provide access to the reply code and message. I mean, if you are wiling to force close and ignore all errors, that context is likely not very relevant anyway.

Happy to update it if it's wanted. With this change it's now just deleting the extension method 😊
Keep in mind that through the interface close method customer could still call an abort wirh custom code. So we wouldn't prevent it

[michaelklishin]

Thank you!

[danielmarbach]

Is it safe to drop the task here because it means we are no longer materializing any exceptions or waiting for the operation to complete

was this only ever called by the upper layers that are now in the extension methods?

[bollhals]

Correct. But I agree that this should probably be fixed in another PR. (I didn't want to mix cleanup and fixing in a pr)

[bollhals]

Also the visibility is why I renamed it to CloseAsync and explicitely dropped the task with _ =. Glad it already worked out😛