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

async/await API #83

Closed
michaelklishin opened this issue May 3, 2015 · 56 comments
Closed

async/await API #83

michaelklishin opened this issue May 3, 2015 · 56 comments
Assignees
Labels
effort-high enhancement next-gen-todo If a rewrite happens, address this issue.
Milestone

Comments

@michaelklishin
Copy link
Member

michaelklishin commented May 3, 2015

Originally this issue was about enabling of message publishing with a natural future ("awaitable" in the .NET world) API.

After some discussions with the .NET community, it makes more sense for us to switch to async/await entirely. This is a major change. The users who cannot upgrade in the meantime will be able to use 3.6.x versions with future RabbitMQ releases.

The decision is to develop a completely new client oriented towards async/await, although #307 introduce it in a place where it makes most sense and can be done with relatively few breaking changes.

@ngbrown
Copy link
Contributor

ngbrown commented Jul 7, 2015

I think moving to an async API with Task returns is a good idea in general, the question is how is this going to be managed?

I've seen .NET libraries (e.g. MongoDB client) that go async do it in a way that splits the entire API into an old/new set of namespaces if not actual libraries (that need to be separately pulled from nuget). Generally, the non-async methods become wrappers for the async ones, thus the split.

@michaelklishin
Copy link
Member Author

That's what we have in mind.

@rasmus
Copy link

rasmus commented Oct 28, 2015

+1

@danielmarbach
Copy link
Collaborator

Could I help out somehow?

@michaelklishin
Copy link
Member Author

@danielmarbach a good way to start would be to look into how much effort it would be to have an async/await-oriented version of IModel and 2 implementations (recovery-aware and "regular" one). Then we can go on to implementing it. All development happens on GitHub in the open.

@danielmarbach
Copy link
Collaborator

Should I do a spike then to see what is affected?

Am 08.01.2016 um 08:46 schrieb Michael Klishin [email protected]:

@danielmarbach a good way to start would be to look into how much effort it would be to have an async/await-oriented version of IModel and 2 implementations (recovery-aware and "regular" one). Then we can go on to implementing it. All development happens on GitHub in the open.


Reply to this email directly or view it on GitHub.

@michaelklishin
Copy link
Member Author

@danielmarbach feel free. Thank you very much for your time!

@danielmarbach
Copy link
Collaborator

I do this the all-in way. No more sync APIs.

Am 08.01.2016 um 11:26 schrieb Michael Klishin [email protected]:

@danielmarbach feel free. Thank you very much for your time!


Reply to this email directly or view it on GitHub.

@michaelklishin
Copy link
Member Author

Sorry but we cannot remove existing APIs. A ton of code in the field depends on them. We can build them on top of async/await but even then I suspect there is no way around having two channel (IModel) APIs.

@danielmarbach
Copy link
Collaborator

Fine with me if you know what the consequences of this decision are:

Having two completely sep. code paths and therefore the potential of fixing one path and forgetting the other one

Am 08.01.2016 um 11:32 schrieb Michael Klishin [email protected]:

Sorry but we cannot remove existing APIs. A ton of code in the field depends on them. We can build them on top of async/await but even then I suspect there is no way around having two channel (IModel) APIs.


Reply to this email directly or view it on GitHub.

@danielmarbach
Copy link
Collaborator

See for example what we did

http://particular.net/blog/async-await-its-time

Am 08.01.2016 um 11:32 schrieb Michael Klishin [email protected]:

Sorry but we cannot remove existing APIs. A ton of code in the field depends on them. We can build them on top of async/await but even then I suspect there is no way around having two channel (IModel) APIs.


Reply to this email directly or view it on GitHub.

@michaelklishin
Copy link
Member Author

@danielmarbach thank you for your perspective. RabbitMQ (and this client) is almost 9 years old. We understand the consequences of major changes, in client libraries and elsewhere. We also know very well what happens when you break things.

.NET client has two versions ("regular" and WinRT) that share effectively all unit tests, for example. So far so good.

@danielmarbach
Copy link
Collaborator

Didn't want to imply that you guys are clueless or anything. Sorry if I sounded like a jerk.

I was talking about the implications on the team maintaining the lib not the breaking changes per se

Am 08.01.2016 um 11:41 schrieb Michael Klishin [email protected]:

@danielmarbach thank you for your perspective. RabbitMQ (and this client) is almost 9 years old. We understand the consequences of major changes, in client libraries and elsewhere. We also know very well what happens when you break things.

.NET client has two versions ("regular" and WinRT) that share effectively all unit tests, for example. So far so good.


Reply to this email directly or view it on GitHub.

@michaelklishin
Copy link
Member Author

No offence taken. We understand that it will be a long and potentially difficult path to get to the new world where everybody uses .NET Core and only C# 5.0+ APIs. We definitely want to get our client there, just without disruptive upgrade-or-leave-it releases.

Also note that certain operations are synchronous by design in the protocol. This does not include publishing or consumption of messages, which is why the issue title only mentions publishing.

@michaelklishin
Copy link
Member Author

By "synchronous" above I mean semantics, not necessarily implementation, of course. Nobody's complained about the blocking nature of IModel.QueueDeclare or QueueBind in my memory, so perhaps those parts of the API make sense as they are.

@danielmarbach
Copy link
Collaborator

Fair enough. You'd have at least the change to base that decision on the shoulders of the whole ecosystem. Many OSS libs and IO bound libs are moving to async only. Just saying ;)

Am 08.01.2016 um 11:48 schrieb Michael Klishin [email protected]:

No offence taken. We understand that it will be a long and potentially difficult path to get to the new world where everybody uses .NET Core and only C# 5.0+ APIs. We definitely want to get our client there, just without disruptive upgrade-or-leave-it releases.

Also note that certain operations are synchronous by design in the protocol. This does not include publishing or consumption of messages, which is why the issue title only mentions publishing.


Reply to this email directly or view it on GitHub.

@danielmarbach
Copy link
Collaborator

@michaelklishin See the effect a proper async implementation would have

#149

@michaelklishin michaelklishin changed the title "Awaitable" publishing with confirms async/await API Jan 8, 2016
@michaelklishin
Copy link
Member Author

I updated the issue to reflect the (greatly) extended scope of this issue.

@danielmarbach
Copy link
Collaborator

Here is a brain dump of a plan of attack:

  • Discuss high-level API and consider getting rid of work service in favour of channels if possible (might need a spike first)
  • Create up-for-grabs issues for indiviual section in the code which can be made async according to the high-level API decisions (to make the work parallizable), use GetAwaiter().GetResult() to isolate these work areas (async changes ripple through pretty quickly)
  • Bring all areas together and remove GetAwaiter().GetResult() calls

Goal:

  • Leave internal infrastructure/design intact if possible but make it async
  • Get rid of synchronous and asynchronous APIs (over events) and make them all Task based

Thoughts? Extensions? Corrections?

@michaelklishin
Copy link
Member Author

There are parts of the API that are naturally events, e.g. consumer cancellation or server-sent connection.close. Those should stay events in my opinion. Event-driven consumers also make sense.

Otherwise, sounds reasonable.

@danielmarbach
Copy link
Collaborator

Ok, but normal event handlers are a PITA with async. We need a better concept

Am 09.01.2016 um 14:42 schrieb Michael Klishin [email protected]:

There are parts of the API that are naturally events, e.g. consumer cancellation or server-sent connection.close. Those should stay events in my opinion. Event-driven consumers also make sense.

Otherwise, sounds reasonable.


Reply to this email directly or view it on GitHub.

@michaelklishin
Copy link
Member Author

How would you handle things with async that can happen at any moment, and are not user-initiated?

@danielmarbach
Copy link
Collaborator

This shows how we could handle async events #151

@SimonCropp
Copy link

@michaelklishin thanks

@danielmarbach
Copy link
Collaborator

Just to restate: I offered my help as well.

Am 05.05.2016 um 10:38 schrieb Michael Klishin [email protected]:

@SimonCropp the only update I have is that we have someone with a lot of .NET experience on our team now and that person will be working on a new client built around async/await. No ETA, no promises.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@michaelklishin
Copy link
Member Author

We've outlined our plan for the future of this client on rabbitmq-users.

@PeterKottas
Copy link

Hello all, very nice to see effort in this direction. Do you have any news on availability of async client? Latest comments are from end of May as far as I know and I was wondering if you have a clearer picture by now. This https://github.com/clearctvm/RabbitMqNext for sure looks great but I would sleep better if we can have it backed up by the community behind rabbit.

@michaelklishin
Copy link
Member Author

@PeterKottas no updates. We've focussed on #148 in June and likely won't start working on the new client until September or so. It will take months to develop anyway.

@PeterKottas
Copy link

Thanks for the info @michaelklishin , I'll check back around the end of the year then.

@michaelklishin
Copy link
Member Author

@PeterKottas I'd check back in Q1-Q2 2017.

@asbjornu
Copy link
Contributor

asbjornu commented Oct 21, 2016

@michaelklishin Just my two cents: @PeterKottas' referenced RabbitMqNext looks like a very decent starting point for an officially supported async capable client. Please consider forking it in some way when this initiative is bootstrapped next year.

Another alternative is https://github.com/EasyNetQ/EasyNetQ, which also has async support.

@michaelklishin
Copy link
Member Author

@asbjornu thank you for the input. Our plan is to develop a client from scratch. That said, we are happy to heavily borrow from existing projects. EasyNetQ is quite different in scope from a "regular" RabbitMQ client, though.

@kjnilsson
Copy link
Contributor

EasyNetQ is a convenience/conventions wrapper over the current .NET client but there may be aspects of the surface API that we could learn from.
RabbitMqNext does have some nice features and implementation details that are well worth considering.
Ultimately it will be a matter of first solidifying what is wanted/needed from a new client and then decide how to best realize that. Async is only one of the aspects (albeit a significant one). We also may need to consider (amongst others):

  • How intuitive the API is to new users.
  • How the API works with other languages on .NET (powershell / fsharp).
  • How easily it can be maintained.
  • How can it be debugged/introspected.
  • How easy it's threading and failure semantics are to reason about.

@asbjornu
Copy link
Contributor

@michaelklishin: Thanks for the info. I agree with @kjnilsson's list of concerns. It would be awesome if the client provided both a low-level API as well as a higher level abstraction such as EasyNetQ. To be honest, the current official client leaves a lot to be desired in terms of user friendliness.

If one's not a die-hard expert on how RabbitMQ works, it's very hard to set it up "right" (where "right" matches 80% of the most common usage patterns) the first time. Properties such as ConnectionFactory.AutomaticRecoveryEnabled not being set to true by default, for instance, has been a common cause of problems in our organization. I would expect these sorts of "sensible defaults" would be set in the higher level abstraction, leaving the lower level to those who are RabbitMQ experts and want to perform tweaks and customizations as close to the metal as possible.

I'm really looking forward to how and where this ends up. 😃

@michaelklishin
Copy link
Member Author

Automatic recovery was introduced less than 2 years ago. Now that we know that it works well in practice (and what kind of issues there were in the Java client, which is a couple of steps ahead of this client in some areas) perhaps it can be enabled by default in a future version. That doesn't have to wait for the new client, though.

@danielmarbach
Copy link
Collaborator

I hope the higher level abstraction will be a dedicated package and not coupled together with the low level driver in one package

Am 27.10.2016 um 21:19 schrieb Asbjørn Ulsberg [email protected]:

@michaelklishin: Thanks for the info. I agree with @kjnilsson's list of concerns. It would be awesome if the client provided both a low-level API as well as a higher level abstraction such as EasyNetQ. To be honest, the current official client leaves a lot to be desired in terms of user friendliness.

If one's not a die-hard expert on how RabbitMQ works, it's very hard to set it up "right" (where "right" matches 80% of the most common usage patterns) the first time. Properties such as ConnectionFactory.AutomaticRecoveryEnabled not being set to true by default, for instance, has been a common cause of problems in our organization. I would expect these sorts of "sensible defaults" would be set in the higher level abstraction, leaving the lower level to those who are RabbitMQ experts and want to perform tweaks and customizations as close to the metal as possible.

I'm really looking forward to how and where this ends up. 😃


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@michaelklishin
Copy link
Member Author

It's unlikely that we will include any abstractions meaningfully higher
level than what the current client has. Existing
projects such as EasyNetQ, NServiceBus and so on fill this niche pretty
well already.

On Fri, Oct 28, 2016 at 12:05 AM, Daniel Marbach [email protected]
wrote:

I hope the higher level abstraction will be a dedicated package and not
coupled together with the low level driver in one package

Am 27.10.2016 um 21:19 schrieb Asbjørn Ulsberg <[email protected]
:

@michaelklishin: Thanks for the info. I agree with @kjnilsson's list of
concerns. It would be awesome if the client provided both a low-level API
as well as a higher level abstraction such as EasyNetQ. To be honest, the
current official client leaves a lot to be desired in terms of user
friendliness.

If one's not a die-hard expert on how RabbitMQ works, it's very hard to
set it up "right" (where "right" matches 80% of the most common usage
patterns) the first time. Properties such as ConnectionFactory.AutomaticRecoveryEnabled
not being set to true by default, for instance, has been a common cause of
problems in our organization. I would expect these sorts of "sensible
defaults" would be set in the higher level abstraction, leaving the lower
level to those who are RabbitMQ experts and want to perform tweaks and
customizations as close to the metal as possible.

I'm really looking forward to how and where this ends up. 😃


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#83 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAEQiNiMLuonPDxM1_9_GSzRudZYcmTks5q4RIIgaJpZM4EOu3x
.

MK

Staff Software Engineer, Pivotal/RabbitMQ

@lasseschou
Copy link

@kjnilsson Just a shout-out from a user looking forward to the TPL-based client. Quick question: Is the progress of the new client available here on GitHub? If so, can you share a link to the repo? Thanks!

@lasseschou
Copy link

I meant @michaelklishin - sorry :)

@michaelklishin
Copy link
Member Author

The work on the new client begins next year.

On 3 Nov 2016, at 07:51, lasseschou [email protected] wrote:

I meant @michaelklishin - sorry :)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@danielmarbach
Copy link
Collaborator

I sent in a PR which would enable opt-in async consumer dispatch in a non-breaking way that can be delivered as a minor, see #307 as soon as I get green light I'll make it a bit more efficient

@gabrieligbastos
Copy link

+1

1 similar comment
@ujeenator
Copy link

+1

@michaelklishin michaelklishin removed their assignment Feb 24, 2017
@VladimirMakaev
Copy link

+1

@rabbitmq rabbitmq locked and limited conversation to collaborators Mar 15, 2017
@michaelklishin
Copy link
Member Author

I'm locking this because it's been decided that we will develop a new async/await-oriented client from scratch. Isolated improvements such as #307 are still possible but no major API revisions are going to happen in this client.

Sorry but we don't need any more +1s.

@lukebakken lukebakken added the next-gen-todo If a rewrite happens, address this issue. label Jan 30, 2020
@lukebakken
Copy link
Contributor

Labeled for next-gen client and closing.

@lukebakken lukebakken added this to the 7.0.0 milestone Dec 29, 2023
@lukebakken lukebakken self-assigned this Dec 29, 2023
@lukebakken
Copy link
Contributor

Version 7 of this library will provide a TPL-based interface.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort-high enhancement next-gen-todo If a rewrite happens, address this issue.
Projects
None yet
Development

No branches or pull requests