search

rabbitmq / rabbitmq-stream-rust-client

A client library for RabbitMQ streams
Other
156 stars 15 forks source link

producer: Safely check if message accumulator is full #188

Closed the-mikedavis closed 1 year ago

the-mikedavis commented 1 year ago

val + 1 could overflow in val == usize::MAX. We can switch the check to val >= self.capacity - 1 because we know that capacity is non-zero: mpsc::channel would panic in MessageAccumulator::new if batch_size were 0.

Connects #186

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.34% :tada:

Comparison is base (f335c59) 86.80% compared to head (b0d9005) 87.15%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #188 +/- ## ========================================== + Coverage 86.80% 87.15% +0.34% ========================================== Files 68 68 Lines 5503 5503 ========================================== + Hits 4777 4796 +19 + Misses 726 707 -19 ``` | [Files Changed](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-rust-client/pull/188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq) | Coverage Ξ” | | |---|---|---| | [src/producer.rs](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-rust-client/pull/188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq#diff-c3JjL3Byb2R1Y2VyLnJz) | `80.04% <100.00%> (ΓΈ)` | | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-rust-client/pull/188/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wolf4ood commented 1 year ago

Hi @the-mikedavis

thanks for the contribution, even though the fix is valid, i think the main issue is that if val == usize::MAX there is something wrong. I did a fix for this in #189 . The main issue was due the spawning of probably too many tasks in a loop.

Give it a look :)

the-mikedavis commented 1 year ago

Ah yeah I agree. I was confused about how val could be usize::MAX, that does seem like something went wrong πŸ˜„

Letting this panic if it reaches usize::MAX might help with future debugging so I will close this out πŸ‘