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.

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

Use randomly generated task IDs to avoid collisions when receiving messages from multiple workers #8708

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Aug 30, 2019

Previously, we were using a monotonically increasing ID that started out with 0. This means there was a high chance of duplicate task IDs being processed at the same time when different execution contexts (aka web workers) sent messages to the another context. Since we store tasks keyed by task ID, tasks that were sent later overwrote tasks with the same ID that were already waiting in the queue. In practice, this only affected messages sent from a worker to the main thread. In most browsers, processing these tasks on the main thread is very quick, so they don't spend a lot of time in the queue. While this bug affects all browsers, Internet Explorer in particular isn't the fastest when processing messages, so had the highest likelihood of message ID collisions actually overwriting tasks waiting in the queue. We didn't observe this bug while testing on CI because we fake the web worker environment there, and task IDs could never be duplicated because they all ran in the same execution context.

This fixes #8702

Needs backport to release-queso.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I think it’s also worth noting why we use random ids in the code comments.

…ssages from multiple workers

Previously, we were using a monotonically increasing ID that started out with 0. This means there was a high chance of duplicate task IDs being processed at the same time when different execution contexts (aka web workers) sent messages to the another context. Since we store tasks keyed by task ID, tasks that were sent later overwrote tasks with the same ID that were already waiting in the queue. In practice, this only affected messages sent from a worker to the main thread. In most browsers, processing these tasks on the main thread is very quick, so they don't spend a lot of time in the queue. While this bug affects all browsers, Internet Explorer in particular isn't the fastest when processing messages, so had the highest likelihood of message ID collisions actually overwriting tasks waiting in the queue. We didn't observe this bug while testing on CI because we fake the web worker environment there, and task IDs could never be duplicated because they all ran in the same execution context.
@mannnick24
Copy link
Contributor

mannnick24 commented Sep 2, 2019

@kkaefer I don't see a test added for this case? or is there another change with test cases updated?
i.e. ensure we get a render with the map loaded in the case of multiple workers

@kkaefer
Copy link
Contributor Author

kkaefer commented Sep 2, 2019

Yes, there's no test case for this because we are currently not set up to test this for several reasons outlined in my comment above:

  • The test runs with Node.js, and not in the browser. Our mock implementation of Web Workers doesn't use distinct threads, so the original problem fixed by this PR doesn't manifest itself there. @arindam1993 is working in Running query tests in browser #8584 to start porting our tests to actually run in the browser environment.
  • The issue is a race condition, and it's hard to reproduce it in a CI environment because it's non-deterministic. We tend to only include tests that are deterministic because non-deterministic tests incur a large number of false positives, which tends to cover true positives.

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

Successfully merging this pull request may close these issues.

IE does not report render event after loaded event
3 participants