Skip to content

Fix racy batching on the dispatcher#2676

Merged
anshulpundir merged 1 commit into
moby:masterfrom
cyli:dispatcher-batch-timer
Jul 6, 2018
Merged

Fix racy batching on the dispatcher#2676
anshulpundir merged 1 commit into
moby:masterfrom
cyli:dispatcher-batch-timer

Conversation

@cyli
Copy link
Copy Markdown
Contributor

@cyli cyli commented Jun 26, 2018

TIL that go timers are racy from #2669. I noticed the dispatcher also calls Reset without ensuring that it's stopped or expired first, so added a similar fix. (See golang/go#11513)

dispatcher/heartbeat.go also has a Reset without calling Stop first, but this seems ok? If an extra heartbeat gets triggered, that seems inexpensive and fine?

Signed-off-by: Ying Li <ying.li@docker.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 26, 2018

Codecov Report

Merging #2676 into master will increase coverage by 0.05%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2676      +/-   ##
==========================================
+ Coverage   62.06%   62.12%   +0.05%     
==========================================
  Files         134      134              
  Lines       21740    21744       +4     
==========================================
+ Hits        13494    13508      +14     
+ Misses       6794     6779      -15     
- Partials     1452     1457       +5

@dani-docker
Copy link
Copy Markdown
Contributor

@cyli
Good catch! this is the second instance #2669 where go timer messes up our logic.
We probably need to review every usage of timer in swarmkit

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Jun 26, 2018

LGTM.

The code as-is should not be a major disruption. The worst case, as far as I can tell, is that we'll call Reset too late and end up immediately launching into another batch processing routine.

@thaJeztah
Copy link
Copy Markdown
Member

ping @anshulpundir PTAL

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants