search

jonboulle / clockwork

a fake clock for golang
Apache License 2.0
656 stars 57 forks source link

Add support for Timers #22

Closed ghost closed 2 years ago

ghost commented 5 years ago

Noticed that #13 appears stale. So I made a new branch locally rebased onto @aviddiviner's work, rebased his work onto master, and addressed the PR comments.

I feel like this code has made the sleeper struct a little obfuscated and could be cleaned up to better reflect what was done for the Ticker stuff. I'm going to try to decouple it myself, but help/suggestions are welcome.

resolves #10

Psykar commented 4 years ago

This actually introduces a rather nasty data race (found during my unit testing)

Read at 0x00c0000b2390 by goroutine 39:
  github.com/jdeal-mediamath/clockwork.(*fakeClock).NewTimer()
      /home/louis/go/pkg/mod/github.com/jdeal-mediamath/clockwork@v0.1.1-0.20190807210122-4545bc8b4b63/clockwork.go:212 +0x74
  github.com/jdeal-mediamath/clockwork.(*fakeClock).After()
      /home/louis/go/pkg/mod/github.com/jdeal-mediamath/clockwork@v0.1.1-0.20190807210122-4545bc8b4b63/clockwork.go:156 +0x42
Previous write at 0x00c0000b2390 by goroutine 38:
  github.com/jdeal-mediamath/clockwork.(*fakeClock).Advance()
      /home/louis/go/pkg/mod/github.com/jdeal-mediamath/clockwork@v0.1.1-0.20190807210122-4545bc8b4b63/clockwork.go:257 +0x467
ghost commented 4 years ago

Can you provide an example to reproduce?

Psykar commented 4 years ago

Hmm. I committed the repo, worked around it with the goal of coming back to it for a proper slimmed down reproduction. Of course now I check out the commit that had the issue, it won't reproduce. Gremlins I tell you.

And looking at the lines in question there doesn't appear to be anything that could be causing a race condition, it's all either local variables or calling functions which have locks on them.

Apologies for the spurious report >.> - appreciate the PR here, has been handy :)

jdgordon commented 4 years ago

@sagikazarmark @jdeal-mediamath any updates on getting this merged? Cheers

sagikazarmark commented 4 years ago

Haven't had time to review it yet. Looks like a race condition has been found in the implementation. Can you share some more details @Psykar ?

Psykar commented 4 years ago

@sagikazarmark I actually dug quite deep into the code at the time and wasn't able to reproduce or see anything that looked like it could cause a race condition - locks and local variables all looked good to me.

ghost commented 3 years ago

I think the original comment on the race condition was later rescinded.

sagikazarmark commented 2 years ago

Fixed in #38