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

Issues with 7.0rc1 IAsyncBasicConsumer's method #1601

Closed
Tornhoof opened this issue Jun 14, 2024 · 4 comments · Fixed by #1602
Closed

Issues with 7.0rc1 IAsyncBasicConsumer's method #1601

Tornhoof opened this issue Jun 14, 2024 · 4 comments · Fixed by #1602
Assignees
Labels
Milestone

Comments

@Tornhoof
Copy link
Contributor

Describe the bug

The new IAsyncBasicConsumer interface has a method HandleBasicDeliver and a method parameter ReadOnlyBasicProperties , but with the modifier in. You can't have in, ref in async methods. So as soon as you implement the interface and actually have anything async to do, i.e. you add the async keyword, it won't compile anymore, with CS1988 (https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/ref-modifiers-errors#reference-variable-restrictions).

Task HandleBasicDeliver(string consumerTag,
ulong deliveryTag,
bool redelivered,
string exchange,
string routingKey,
in ReadOnlyBasicProperties properties,
ReadOnlyMemory<byte> body);

in basically makes only sense together with ref struct and specifically ref readonly struct, atleast that's what I understand from all the discussions around https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-11.0/low-level-struct-improvements.

I highly recommend simply removing in this prior to the 7.0 release, as modifying it would be a breaking change and at the moment implementations would need to jump through loops to use it with async (async local method etc.).

In IBasicConsumer it's not there:

Task HandleBasicDeliverAsync(string consumerTag,
ulong deliveryTag,
bool redelivered,
string exchange,
string routingKey,
ReadOnlyBasicProperties properties,
ReadOnlyMemory<byte> body);

Reproduction steps

See for example:
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gVgG4biBmJ0hgYQYBvGgzFMexABxMkDALIBPeTAwALCABMAFAEsAdgwAqMXBgDKGKAFcwGBvkUA1bABtrMAJSjxQ8eJ8xAF9AhlDuBlhsTQh9V0UGMxs7Y1MLK1sMUJFqfxDqIKA===

Expected behavior

Additional context

No response

@Tornhoof Tornhoof added the bug label Jun 14, 2024
@michaelklishin
Copy link
Member

Thank you for providing all the details from the start!

@lukebakken
Copy link
Contributor

Great, this will be easy to fix. Feel free to open a PR yourself if you'd like!

@lukebakken lukebakken self-assigned this Jun 14, 2024
@lukebakken lukebakken added this to the 7.0.0 milestone Jun 14, 2024
@Tornhoof
Copy link
Contributor Author

will do.

@bollhals
Copy link
Contributor

The problem with this is, ReadOnlyBasicProperties is a rather large struct, hence the in modifier was added back when it wasn't async yet to avoid copying the struct.

Now that the library moved to async, in is problematic as you've seen.
So the use of a large struct for these API's might actually be worse than creating a class to hold them.

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

Successfully merging a pull request may close this issue.

4 participants