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.

Sign up for GitHub

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

Jump to bottom

Fix race conditions #215

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix race conditions #215

wants to merge 9 commits into from

Conversation

kairichard
Copy link
Contributor

@kairichard kairichard commented May 5, 2019 •
edited
Loading

This PR fixes all race conditions found by -race on go test and consequently adds -race to the .circleci.

  • port now exposes mutex.Lock/Unlock
  • port can now detect a closed channel although this can cause panics when forwarding a value
  • we nolonger leak goroutines after op.Stop

Kai Richard Koenig added 2 commits May 5, 2019 17:30
Fix race condition where done could be written to and read from
c0766f5
Allow operator goroutines to finish.
cb4a5a4 
This was rather hard to track down, but what I noticed
was hanging/leaking goroutines waiting on `<-p.buf`. This came up during hunting
race conditions.

What happened here was that the gorouting that executed `opFunc` could
never return because the operator was somewhere trying to `Pull`. Which
in turn meant waiting on a message recievied `<-p.buf`. Altough we have
called `Stop` on the operator.

This fix now closes all ports resulting in more leaked goroutines.
@kairichard kairichard added the WIP label May 5, 2019
@codecov-io
Copy link

codecov-io commented May 5, 2019 •
edited
Loading

Codecov Report

Merging #215 into master will increase coverage by 0.4%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #215     +/-   ##
=========================================
+ Coverage   40.99%   41.39%   +0.4%     
=========================================
  Files         100      100             
  Lines        5472     5496     +24     
=========================================
+ Hits         2243     2275     +32     
+ Misses       3049     3043      -6     
+ Partials      180      178      -2
Impacted Files Coverage Δ
pkg/elem/meta_store.go 92.47% <100%> (+0.7%) ⬆️
pkg/elem/control_reduce.go 100% <100%> (ø) ⬆️
pkg/core/operator.go 81.98% <100%> (+1.52%) ⬆️
pkg/core/port.go 71.78% <59.09%> (+1.23%) ⬆️
pkg/elem/control_take.go 66.12% <0%> (+3.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 724413a...6cc7c31. Read the comment docs.

@kairichard kairichard changed the title WIP: Fix race conditions Fix race conditions May 5, 2019
@kairichard kairichard requested a review from jm9e May 5, 2019 16:41
@kairichard kairichard added concept and removed WIP labels May 5, 2019
@td5r td5r removed the concept label May 10, 2019
@kairichard
Copy link
Contributor Author

@jm9e Very intresting, there race isn't fixed as it now appears randomly, where 58773ac failed, although being clean and 692f0a5 passing. I strongly belive this has something to do with sending the ´zero value´ from the closed channel to another operator. One way of moving forwared IMO would be to introduce another marker. Another one could be to investigate why this seems to only happen to merge_sort and potentionally find the root cause.

@kairichard
Copy link
Contributor Author

Don't be fooled by 6cc7c31 passing as we have seen others commits failing. It appears to me that this is a flapping test which cannot ATM be reproduced in a predictable way.

@td5r
Copy link
Contributor

td5r commented Jun 5, 2019 •
edited
Loading

@jm9e can this be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jm9e jm9e Awaiting requested review from jm9e

At least 1 approving review is required to merge this pull request.

Assignees

@jm9e jm9e

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kairichard @codecov-io @td5r @jm9e