Skip to content

Commit 0dd7f20

Browse files
Copybara Servicecopybara-github
authored andcommitted
Fix race conditions in polling client communicator Flush and Reset
Fixes concurrency and synchronization issues in the polling client communicator around Flush() and Reset() while preserving the original scheduling, throttling, and lifecycle behaviors: 1. Replaces the single-consumer `pollComplete` channel with a broadcast channel closure mechanism (`pollDone chan struct{}`), ensuring multiple concurrent `Flush()` callers all wake up properly without channel read contention. 2. Replaces `pollDone` on `Reset()` and outbox enqueue to ensure `Flush()` waits for the appropriate poll cycle. 3. Differentiates `Reset()`-induced request cancellations from network failures (`wasReset`), preserving queued messages for immediate re-poll rather than NACKing them. 4. Preserves `lastActive` dynamic latency reduction, `oldestUnsent` buffer delay tracking, `FailureSuicideTimeSeconds` suicide timeout, and shutdown flush behavior. PiperOrigin-RevId: 949430003
1 parent 2cf530c commit 0dd7f20

1 file changed

Lines changed: 66 additions & 30 deletions

File tree

fleetspeak/src/client/https/polling.go

Lines changed: 66 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,19 @@ type Communicator struct {
5959

6060
certBytes []byte
6161

62-
wakeUp chan struct{}
63-
pollComplete chan error
64-
mu sync.Mutex
65-
pollCancel context.CancelFunc
62+
// Synchronization for Reset/Flush
63+
wakeUp chan struct{}
64+
mu sync.Mutex
65+
pollDone chan struct{}
66+
lastPollErr error
67+
pollCancel context.CancelFunc
6668
}
6769

6870
// Setup implements comms.Communicator.
6971
func (c *Communicator) Setup(cl comms.Context) error {
7072
c.cctx = cl
73+
c.pollDone = make(chan struct{})
74+
close(c.pollDone) // Start closed so Flush returns immediately if no messages.
7175
return c.configure()
7276
}
7377

@@ -114,7 +118,6 @@ func (c *Communicator) configure() error {
114118
c.ctx, c.done = context.WithCancel(context.Background())
115119
c.clientCertificateHeader = si.ClientCertificateHeader
116120
c.wakeUp = make(chan struct{}, 1)
117-
c.pollComplete = make(chan error, 1)
118121
c.certBytes = certBytes
119122
return nil
120123
}
@@ -141,13 +144,9 @@ func (c *Communicator) Reset() {
141144
if c.pollCancel != nil {
142145
c.pollCancel()
143146
}
147+
c.pollDone = make(chan struct{})
144148
c.mu.Unlock()
145149
c.hc.Transport.(*http.Transport).CloseIdleConnections()
146-
// Drain pollComplete to ensure Flush waits for a new poll.
147-
select {
148-
case <-c.pollComplete:
149-
default:
150-
}
151150
select {
152151
case c.wakeUp <- struct{}{}:
153152
default:
@@ -158,10 +157,23 @@ func (c *Communicator) Reset() {
158157
func (c *Communicator) Flush(ctx context.Context) error {
159158
log.InfoContextf(ctx, "Flush called")
160159
for {
160+
c.mu.Lock()
161+
done := c.pollDone
162+
c.mu.Unlock()
163+
161164
select {
162165
case <-ctx.Done():
163166
return ctx.Err()
164-
case err := <-c.pollComplete:
167+
case <-c.ctx.Done():
168+
return c.ctx.Err()
169+
case <-done:
170+
c.mu.Lock()
171+
err := c.lastPollErr
172+
if err != nil && c.pollDone != done {
173+
c.mu.Unlock()
174+
continue
175+
}
176+
c.mu.Unlock()
165177
return err
166178
}
167179
}
@@ -193,25 +205,48 @@ func (c *Communicator) processingLoop() {
193205
// for the MinFailureDelay.
194206
poll := func() {
195207
var err error
208+
c.mu.Lock()
209+
select {
210+
case <-c.pollDone:
211+
c.pollDone = make(chan struct{})
212+
default:
213+
}
214+
reqCtx, cancel := context.WithCancel(c.ctx)
215+
c.pollCancel = cancel
216+
myDone := c.pollDone
217+
c.mu.Unlock()
218+
196219
defer func() {
197-
select {
198-
case c.pollComplete <- err:
199-
default:
200-
}
220+
cancel()
221+
c.mu.Lock()
222+
c.pollCancel = nil
223+
c.lastPollErr = err
224+
close(myDone)
225+
c.mu.Unlock()
201226
}()
227+
202228
c.wd.Reset()
203229
if c.cctx.CurrentID() != c.id {
204230
c.configure()
205231
}
206232
var active bool
207-
active, err = c.poll(toSend)
233+
active, err = c.poll(reqCtx, toSend)
208234
if err != nil {
235+
c.mu.Lock()
236+
wasReset := errors.Is(err, context.Canceled) && c.ctx.Err() == nil
237+
c.mu.Unlock()
238+
239+
if wasReset {
240+
return
241+
}
242+
209243
log.Warningf("Failure during polling: %v", err)
210244
for _, m := range toSend {
211245
m.Nack()
212246
}
213247
toSend = nil
214248
toSendSize = 0
249+
oldestUnsent = time.Time{}
215250

216251
if (!lastPoll.IsZero()) && (time.Since(lastPoll) > time.Duration(c.conf.FailureSuicideTimeSeconds)*time.Second) {
217252
// Die in the hopes that our replacement will be better configured, or otherwise have better luck.
@@ -310,6 +345,14 @@ func (c *Communicator) processingLoop() {
310345
poll()
311346
case m := <-c.cctx.Outbox():
312347
t.Stop()
348+
c.mu.Lock()
349+
select {
350+
case <-c.pollDone:
351+
c.pollDone = make(chan struct{})
352+
default:
353+
}
354+
c.mu.Unlock()
355+
313356
toSend = append(toSend, m)
314357
toSendSize += 2 + proto.Size(m.M)
315358
if toSendSize >= sendBytesThreshold ||
@@ -324,7 +367,7 @@ func (c *Communicator) processingLoop() {
324367
}
325368
}
326369

327-
func (c *Communicator) poll(toSend []comms.MessageInfo) (bool, error) {
370+
func (c *Communicator) poll(ctx context.Context, toSend []comms.MessageInfo) (bool, error) {
328371
var sent bool // records whether an interesting (non-LOW) priority message was sent.
329372
msgs := make([]*fspb.Message, 0, len(toSend))
330373
for _, m := range toSend {
@@ -347,9 +390,12 @@ func (c *Communicator) poll(toSend []comms.MessageInfo) (bool, error) {
347390
}
348391

349392
for i, host := range c.hosts {
350-
cd, err := c.pollHost(host, data)
393+
cd, err := c.pollHost(ctx, host, data)
351394
if err != nil {
352395
log.Warningf("Error polling %q for ContactData: %v", host, err)
396+
if ctx.Err() != nil {
397+
return false, ctx.Err()
398+
}
353399
continue
354400
}
355401
if i != 0 {
@@ -367,7 +413,7 @@ func (c *Communicator) poll(toSend []comms.MessageInfo) (bool, error) {
367413
return false, errors.New("unable to contact any server")
368414
}
369415

370-
func (c *Communicator) pollHost(host string, data []byte) (*fspb.ContactData, error) {
416+
func (c *Communicator) pollHost(ctx context.Context, host string, data []byte) (*fspb.ContactData, error) {
371417
var sendErr, recvErr error
372418
var sendSize, recvSize int
373419
defer func() {
@@ -396,17 +442,7 @@ func (c *Communicator) pollHost(host string, data []byte) (*fspb.ContactData, er
396442
if sendErr != nil {
397443
return nil, sendErr
398444
}
399-
var reqCtx context.Context
400-
c.mu.Lock()
401-
reqCtx, c.pollCancel = context.WithCancel(c.ctx)
402-
c.mu.Unlock()
403-
defer func() {
404-
c.mu.Lock()
405-
c.pollCancel()
406-
c.pollCancel = nil
407-
c.mu.Unlock()
408-
}()
409-
req = req.WithContext(reqCtx)
445+
req = req.WithContext(ctx)
410446
SetContentEncoding(req.Header, c.conf.GetCompression())
411447
if c.clientCertificateHeader != "" {
412448
bc := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: c.certBytes})

0 commit comments

Comments
 (0)