go-zeromq / zmq4

[WIP] Pure-Go implementation of ZeroMQ-4
BSD 3-Clause "New" or "Revised" License
341 stars 56 forks source link

Fix rep cancellation #143

Closed encse closed 1 year ago

encse commented 1 year ago

I have found a race condition in the ctx cancellation in repWriter.

Added a test which reproduces the issue with a possible fix.

I don't quite understand why dont we use ctx in the write routine, see my second commit about that.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 71.42% and project coverage change: -0.22% :warning:

Comparison is base (bd7e871) 67.63% compared to head (099cec7) 67.42%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #143 +/- ## ========================================== - Coverage 67.63% 67.42% -0.22% ========================================== Files 29 29 Lines 2555 2560 +5 ========================================== - Hits 1728 1726 -2 - Misses 724 730 +6 - Partials 103 104 +1 ``` | [Files Changed](https://app.codecov.io/gh/go-zeromq/zmq4/pull/143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-zeromq) | Coverage Δ | | |---|---|---| | [rep.go](https://app.codecov.io/gh/go-zeromq/zmq4/pull/143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-zeromq#diff-cmVwLmdv) | `69.43% <71.42%> (-0.26%)` | :arrow_down: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/go-zeromq/zmq4/pull/143/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-zeromq)

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

sbinet commented 1 year ago

(apologies for the belated answer: holidays and the backlog from work...)

thanks for the PR.

could you send another PR against https://github.com/go-zeromq/license adding yourself to the AUTHORS and/or CONTRIBUTORS files ? (then I'll merge that one. it will take less than a month from side :P)

encse commented 1 year ago

Hey, thanks for your time. I added myself in this PR: https://github.com/go-zeromq/license/pull/16

sbinet commented 1 year ago

needs https://github.com/go-zeromq/license/pull/16

sbinet commented 1 year ago

thanks a lot.