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

Remove TODO #1516

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Remove TODO #1516

merged 1 commit into from
Mar 13, 2024

Conversation

lukebakken
Copy link
Contributor

Add assertion

@lukebakken lukebakken added this to the 7.0.0 milestone Mar 11, 2024
@lukebakken lukebakken self-assigned this Mar 11, 2024
@lukebakken
Copy link
Contributor Author

lukebakken commented Mar 12, 2024

@stebet I hope you have some time to comment about this. When reviewing the pipelines code you added to this library (thank you again!) I noticed that the IsSingleSegment property of the PipeReader's buffer was not checked and thus the code assumes that the buffer is a single segment. I only thought to check this due to reading @mgravell's blog posts about `System.IO.Pipelines.

So, in this PR, I added an assertion that the buffer is a single segment:

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/lukebakken/todos/projects/RabbitMQ.Client/client/impl/Frame.cs#L317

...and, sure enough, the assertion does not always hold:

https://github.com/rabbitmq/rabbitmq-dotnet-client/actions/runs/8241012258/job/22537494233

What's interesting is that running that test in isolation, on my local machine, does not reproduce the assertion failure. Only running the entire suite does.

I'm going to see if I can handle this case efficiently, but I would love a second opinion.

cc-ing @bording and @danielmarbach in case they have some input.

UPDATE: I think I answered my own question:
b680b90

@bording
Copy link
Collaborator

bording commented Mar 12, 2024

@lukebakken A quick look at the section of code in question, I think it's fine because there isn't an optimized path that's doing something different when there is a single segment (no place that assumes buffer.First is the entire set of data).

@stebet
Copy link
Contributor

stebet commented Mar 12, 2024

Good catch @lukebakken!

Edit: Just saw the explanation :)

Add assertion

Remove assertion, add comment

* Add `ConfirmsAreEnabled` in `ChannelBase`

* Add `CancellationToken` to `CreateChannelAsync`

* Use `CancellationToken` argument in `OpenAsync`
@lukebakken lukebakken marked this pull request as ready for review March 13, 2024 17:53
@lukebakken lukebakken merged commit 0866269 into main Mar 13, 2024
11 checks passed
@lukebakken lukebakken deleted the lukebakken/todos branch March 13, 2024 18:26
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 this pull request may close these issues.

3 participants