Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

flush: guarantee that all inflight messages are sent #353

Merged
merged 6 commits into from
Nov 16, 2022

Conversation

edsonjab
Copy link
Contributor

@edsonjab edsonjab commented Nov 9, 2022

Simple fix so flush waits for previous inflight messages to be sent.

Resolves #309

@yo1dog
Copy link

yo1dog commented Nov 17, 2022

This does not resolve #309 as there are still multiple race conditions in this solution.

Did you really not even bother to test if the solution fixed at least the example scenarios?

The original example from #309 still fails:

const analytics = new Analytics(writeKey, {flushAt: 2});
analytics.track(...); // This will trigger a flush (flush A) because line 215: flushed === false
// pendingFlush is now set to flush A's promise.
analytics.track(...);
analytics.track(...); // This will trigger a flush (flush B) because line 221: queue.length >= flushAt
// flush B awaits pendingFlush (flush A's promise)

// The following will trigger a flush (flush C) because there is a message in the queue.
// (Flush B is still awaiting flush A and has not emptied the queue yet.)
// Flush C awaits pendingFlush (flush A's promise - NOT flush B's promise)
analytics.flush().then(() => {
  // This calllback may be called before flush B completes.
  // The process may exit while the message from flush B is still inflight.
  console.log('done');
  process.exit(0);
});
// Once flush A's promise resolves, there are now 2 concurrent flushes (B and C) running in parallel.
// Further, flush C contains 0 events.
// Waiting on flush C creates a race condition as it may finish before flush B.

And this slightly modified version of the second example from #309 still fails:

const analytics = new Analytics(writeKey, {flushAt: 2});
analytics.track(...); // This will trigger a flush (flush A) because line 215: flushed === false
// pendingFlush is now set to flush A's promise.
analytics.track(...);
analytics.track(...); // This will trigger a flush (flush B) because line 221: queue.length >= flushAt
// Flush B awaits pendingFlush (flush A's promise)
analytics.track(...);
// The following will trigger a flush (flush C) because there is a message in the queue.
// Flush C awaits pendingFlush (flush A's promise - NOT flush B's promise)
analytics.flush().then(() => {
  // This calllback may be called before flush B completes.
  // The process may exit while the message from flush B is still inflight.
  console.log('done');
  process.exit(0);
});
// Once flush A's promise resolves, there are now 2 concurrent flushes (B and C) running in parallel.
// Waiting on flush C creates a race condition as it may finish before flush B.

Here is an even simpler example:

const analytics = new Analytics(writeKey);
analytics.flushed = true;
analytics.track('A');
analytics.flush();
analytics.track('B');
analytics.flush();
analytics.track('C');
await analytics.flush();
process.exit(0);
  1. Flush A empties the queue and sets pendingFlush.
  2. Flush B and C await Flush A's promise.
  3. A's promise resolves.
  4. B's flush continues, empties the queue, and sets pendingFlush.
  5. C's flush continues, empties the queue (it's already empty), and overwrites pendingFlush.
  6. Flush B and C are now occurring in parallel but only flush C is awaited, creating a race condition in which the process can exit before flush B completes. Further, events B and C and sent via flush B and flush C is empty.

@Jackman3005
Copy link

Can we get this added as a proper test case and ensure it is covered?

Having a flush function that does not work correctly is worse than not having it at all.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flush does not guarantee that all inflight messages are sent
6 participants