Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-review use of Task vs ValueTask in API #1645

Closed
lukebakken opened this issue Jul 30, 2024 · 8 comments · Fixed by #1646
Closed

Re-review use of Task vs ValueTask in API #1645

lukebakken opened this issue Jul 30, 2024 · 8 comments · Fixed by #1646
Assignees
Milestone

Comments

@lukebakken
Copy link
Contributor

lukebakken commented Jul 30, 2024

See the discussion here: #1642

cc @danielmarbach

@lukebakken lukebakken added this to the 7.0.0 milestone Jul 30, 2024
@lukebakken lukebakken self-assigned this Jul 30, 2024
lukebakken added a commit that referenced this issue Jul 30, 2024
Fixes #1645

* Start with `IFrameHandler` as pointed out by @danielmarbach
lukebakken added a commit that referenced this issue Jul 31, 2024
Fixes #1645

* Start with `IFrameHandler` as pointed out by @danielmarbach
lukebakken added a commit that referenced this issue Aug 6, 2024
Fixes #1645

* Start with `IFrameHandler` as pointed out by @danielmarbach
@lukebakken lukebakken reopened this Sep 5, 2024
@lukebakken
Copy link
Contributor Author

@danielmarbach @stebet @paulomorgado @Hawxy

Re: Task vs ValueTask in the public API - my current inclination is to just use Task unless someone can come up with a concrete benchmark showing an appreciable performance benefit for ValueTask, or provide another compelling reason to use ValueTask.

As you are all aware, version 7 has been years in the making. I am going to ship version 7 this month, no matter what 😸

@danielmarbach
Copy link
Collaborator

I definitely don't want to derail the release @lukebakken

Just sharing thoughts and observations from our upgrade attempts. At the end of the day, if we leave things as is, that's also fine.

@lukebakken
Copy link
Contributor Author

Just sharing thoughts and observations from our upgrade attempts.

I'm all for making migrations from 6.x easier.

@danielmarbach
Copy link
Collaborator

Well one of the biggest pain points was that the order of some parameters changed and not the value task vs task 😜

@lukebakken
Copy link
Contributor Author

Well one of the biggest pain points was that the order of some parameters changed

I'm planning to update the tutorials to use the latest RC before shipping version 7, so I'll probably make note of this, but if you'd like to mention which changes were most annoying that would be great. Maybe I can restore the parameter order in the API.

@danielmarbach
Copy link
Collaborator

@lukebakken the entire diff is visible here Particular/NServiceBus.RabbitMQ#1446

@lukebakken
Copy link
Contributor Author

@danielmarbach it looks like the BasicPublishAsync API was the biggest (maybe only?) offender, so I reverted the parameter order change in this commit: 5abd912

@lukebakken
Copy link
Contributor Author

Based on the comment by @Hawxy -

#1413 (comment)

...I re-visited IChannel. Those methods that directly call ModelSendAsync without an RPC continuation will return ValueTask, while everything else returns Task or Task<>. Thanks everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants