7.0 release checklist / misc items

[lukebakken]

• Ship Public API in txt file
• Update CHANGELOG.md
• Update copyright headers (Update copyright message #1589)
• Use newer C# language features
  • readonly fields instead of get;
• Consider immutable collections, and / or make as many object immutable via language features as possible. See @danielmarbach's comment here
• Address TODO comments in the code
• Expose public implementation of ITcpClient or come up with a better way of setting socket options. See Removed ReceiveBufferSize and SendBufferSize to improve message rates #1415 and this discussion. Note: TcpClientAdapter was made public in Make TcpClientAdapter public #1417 (UPDATE: this may be a FUTURE item)
• Ensure documentation XML is accurate
• Remove deprecated API methods and other entities. API should be async-only.
• Replace var with explicit types when appropriate.
• Investigate "bypassing" await when IsCompletedSuccessfully is true (though, frankly, I prefer easier-to-read code 😸). These appear to be relevant:
  • await ValueTask performance dotnet/runtime#11803
  • await ValueTask performance dotnet/runtime#11803 (comment)
  • https://blog.stephencleary.com/2016/12/eliding-async-await.html
• Do not "elide" async/await when exceptions must be caught (@danielmarbach's noticed that here)
• See if exception handling code can be simplified (see @Tornhoof's comment)
• Ensure convenience methods like QueueDeclarePassive have async versions.

[lukebakken]

@stebet what do you think of this item?

Mark all sync API methods as Obsolete. Version 8 of this lib could then be async-only.

[stebet]

@stebet what do you think of this item?

Mark all sync API methods as Obsolete. Version 8 of this lib could then be async-only.

All for it!

[bangjiehan]

Does we need QueueDeclarePassiveAsync too?

[lukebakken]

@bangjiehan -

Does we need QueueDeclarePassiveAsync too?

The QueueDeclareAsync method has a passive parameter. Set it to true, and you will do a passive declaration.

QueueDeclarePassive has always been a convenience method added to the API. I'll make a note to add it to version 7.

[lukebakken]

#1589

[Gladskih]

Is there documentation for new API? Or at least some example.

[lukebakken]

@Gladskih there isn't too much of a difference between the old API and the new one, to be honest. If you need a reference, the best place at this time are the test applications and integration tests:

• https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/Test/Applications/MassPublish/Program.cs
• https://github.com/rabbitmq/rabbitmq-dotnet-client/tree/main/projects/Test/Integration

If you need further assistance, please start a discussion rather than commenting on an issue:
https://github.com/rabbitmq/rabbitmq-dotnet-client/discussions

[YarinOmesi]

@bollhals just FYI, I have decided that version 7 should ship on July 12th. I'm releasing 7.0.0-rc.2 today.

Any news on the release date ?

[lukebakken]

@YarinOmesi - if you'd like to assist with the release of version 7, test out an RC and report back how it works in your environment.

[lukebakken]

@bollhals - thanks for your latest contribution (#1636). Anything else on your plate for version 7? I am going to release a new RC today.

[bollhals]

@bollhals - thanks for your latest contribution (#1636). Anything else on your plate for version 7? I am going to release a new RC today.

Yes maybe. I'm almost done working on removing the non async @workflow for e.g. IBasicConsumer / Consumedispatcher. )
As this is facing public api change, it might be beneficial to include in 7.O

[lukebakken]

@bollhals feel free to open a draft PR with the work you have already done for that.

[bollhals]

Will do once I have something that is buildable / testable.

[Hawxy]

if you'd like to assist with the release of version 7, test out an RC and report back how it works in your environment.

@lukebakken Just some feedback as a developer of Wolverine, I upgraded our vnext branch to v7 RC a few weeks ago and all of our tests are passing. RC2 was a bit broken for us, but RC3 onwards has been fine.

[lukebakken]

@Hawxy thank you!

[danielmarbach]

I noticed some of the channel extensions don't have cancellation token support. I sent in a PR #1641

There seems to be also connection extensions that might require a cancellation token (the ones that don't accept a timespan but don't have a token yet)

[danielmarbach]

I have also noticed some API asymmetry, and I don't understand why that is the case.

BasicAck and Nack return ValueTask while BasicReject returns a Task and has to call AsTask on ModelSendAsync which makes we wonder whether this was deliberate or the client API hasn't been properly aligned where necessary?

[lukebakken]

@danielmarbach I'll wait on releasing version 7 since you're finding stuff to address. Plus, there is #1640 to deal with as well. Thank you!

[danielmarbach]

@lukebakken I was wondering if it would make sense to consistently use value task on the channel APIs

Currently it is a mix and that leads to different ripple effects of having to adjust the outer code depending on the method you call.

Technically the Value Task on the public API would leave room for future optimizations under the hood without having to break the API. I guess the tradeoff there is API consistency and room for future changes under the cover.

https://blog.marcgravell.com/2019/08/prefer-valuetask-to-task-always-and.html?m=1

Thoughts? @stebet @paulomorgado @bollhals

[Hawxy]

https://blog.marcgravell.com/2019/08/prefer-valuetask-to-task-always-and.html?m=1

Worth mentioning that this blog post is out of date. The value task pooling mentioned was a .NET 5 experiment & never shipped as it negatively impacted performance in some scenarios and is now opt-in via an attribute (you'd need to benchmark to see if anything benefits from it). The recommendation of using ValueTask by default to benefit from some magical future improvement is incorrect.

The general rule of thumb is:

• Use ValueTask if all downstream callers are also using ValueTask (all calls down to System.Threading.Channels makes sense).
• OR use ValueTask if an execution path is going to return synchronously most of the time (ie a GetOrSetAsync caching method).

[danielmarbach]

Just to clarify. By default I'm also in the deliberate usage of Value Task camp. I only brought this up because as a user of the API the mixed mode has consequences too and changing the return value is a breaking change

[paulomorgado]

I'll have to admit that I haven't delved myself into the benefits of using ValueTask/ValueTask<T>.

@mgravell might be able to shed some light in how he feels about it 5 years later.

There's also Understanding the Whys, Whats, and Whens of ValueTask, from @stephentoub.

ValueTask/ValueTask<T> is better than Task/Task<T> for synchronous completion but comes with a few limitations over Task/Task<T> like being awaitable only once and sync-over-async might not work as expected.

ValueTask/ValueTask<T> can be pooled by explicitly using a specific state machine builder.

I would look into .NET source code the gather insights on real usage, but there's probably not time for that.

ValueTask/ValueTask<T>, although feeling strange, might be more future-proof. But that's just a feeling at this time.

[stebet]

I'm in the prefer ValueTask camp as well. There was a good blog post about it here: https://blog.marcgravell.com/2019/08/prefer-valuetask-to-task-always-and.html?m=1

[danielmarbach]

Related to @Hawxy and my comments and the guidance shared

When adding abstract, virtual, or interface methods, you also need to consider whether these situations will exist for overrides/implementations of that method.

https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/

is the type of nuanced discussion I also wanted to kick off here before the API is shipped.

[lukebakken]

@danielmarbach @stebet @paulomorgado @Hawxy please move the Task / ValueTask discussion here -

#1645

Thanks.

[lukebakken]

• Double-check the public API file
• Update the changelog