fix a race condition in PipeListener #21

Merged
merged 1 commit into from Jun 21, 2016

Conversation

Projects
None yet
1 participant
Owner

natefinch commented Jun 21, 2016

There was an obvious race between PipeListener's Accept and Close modifying l.closed, found with go test -race, and a less obvious race that if someone calls Close before we call connectnamedpipe, the connect would hang forever. Now we have a lock over the whole of Accept and Close (except while doing WaitSingleObject, which will corrently return an error when we Close), so Close can't screw up connect named pipe.

Owner

natefinch commented Jun 21, 2016

Testing done: there was a test in juju (under worker/metrics/sender) that would occasionally hang (approx 1/10 times)... with this fix, no more hang in 300 trials.

(also go test -race used to find a race condition and now does not)

fix a race condition
There was an obvious race condition where both Accept and Close were
twiddling with PipeListener.close.. but there was also a less obvious race
cnodition where if you called Close between createpipe and
connectnamedpipe, the connect would hang forever. These are both now fixed
by making the mutex protect the entire accept/close methods (aside from
waitsingleobject, which will correctly error out if you call close while
it waits).

@natefinch natefinch merged commit c1b8fa8 into v2 Jun 21, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

jujubot added a commit to juju/juju that referenced this pull request Jun 21, 2016

Merge pull request #5675 from natefinch/fix-1581157
Fix Windows Test Hang

This fixes https://bugs.launchpad.net/juju-core/+bug/1581157 by removing a race condition
that occurred when Close was called concurrently with Accept.

(Review request: http://reviews.vapour.ws/r/5119/)

Corresponding fix in npipe: natefinch/npipe#21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment