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

Fix queue management in jest-worker #7934

Merged
merged 1 commit into from Feb 19, 2019
Merged

Conversation

@mjesun
Copy link
Contributor

mjesun commented Feb 19, 2019

Nodes in a linked list were shared among all lists for each worker; which is not what we would like to do. The expected part to be shared is only the request object (which uses the first position as a lock between jobs); but the wrapper object for the linked list needs to be unique.

I also added a test that would crash without the fix.

@mjesun mjesun force-pushed the mjesun:fix-worker-queues branch from c7abe65 to 0af378c Feb 19, 2019
@mjesun mjesun requested review from rickhanlonii and SimenB Feb 19, 2019

return this;
}

private _enqueue(task: QueueChildMessage, workerId: number): Farm {
const item = {task, next: null};

This comment has been minimized.

Copy link
@mjesun

mjesun Feb 19, 2019

Author Contributor

The core of the fix is only this line; where instead of pushing to all queues the task object, we create one specifically for each queue, (meaning that when doing .next in the tail of a linked list, we don't affect all others).

The rest of the PR are "cosmetic" changes, and the test.

@mjesun mjesun force-pushed the mjesun:fix-worker-queues branch from 0af378c to 0297b72 Feb 19, 2019
@mjesun mjesun requested a review from rubennorte Feb 19, 2019
this._offset++;

return this;
}

lock(workerId: number): void {
private _lock(workerId: number): void {

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 19, 2019

Collaborator

technically breaking, but I guess these shouldn't be called anyways by consumers?

This comment has been minimized.

Copy link
@mjesun

mjesun Feb 19, 2019

Author Contributor

Yeah, playing with worker locks is a recipe for disaster :D

@@ -14,7 +14,8 @@
- `[jest-changed-files]` Improve default file selection for Mercurial repos ([#7880](https://github.com/facebook/jest/pull/7880))
- `[jest-validate]` Fix validating async functions ([#7894](https://github.com/facebook/jest/issues/7894))
- `[jest-circus]` Fix bug with test.only ([#7888](https://github.com/facebook/jest/pull/7888))
- `[jest-transform]` Normalize config and remove unecessary checks, convert `TestUtils.js` to TypeScript ([#7801](https://github.com/facebook/jest/pull/7801)
- `[jest-transform]` Normalize config and remove unecessary checks, convert `TestUtils.js` to TypeScript ([#7801](https://github.com/facebook/jest/pull/7801))

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 19, 2019

Collaborator

took me forever to spot the fixed missing paren :P

@SimenB
SimenB approved these changes Feb 19, 2019
@mjesun mjesun merged commit 7cc0771 into facebook:master Feb 19, 2019
10 of 12 checks passed
10 of 12 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
facebook.jest in progress
Details
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
private _locks: Array<boolean>;
private _numOfWorkers: number;
private _offset: number;
private _queue: Array<QueueChildMessage | null>;
private _queue: Array<QueueItem | null>;

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 19, 2019

Collaborator
Suggested change
private _queue: Array<QueueItem | null>;
private _queue: Array<QueueItem>;

right? there can never be nulls in the queue now?

This comment has been minimized.

Copy link
@mjesun

mjesun Feb 19, 2019

Author Contributor

The queue can be empty, and that's signaled by a null item.

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 19, 2019

Collaborator

so Array<QueueItem> | [null]? I guess it doesn't matter in practice

This comment has been minimized.

Copy link
@mjesun

mjesun Feb 19, 2019

Author Contributor

The array index refers to each of the workers (I.e. this._queue.length is numOfWorkers, not the list of jobs); so each item in the array is either the first element of the linked list, or null.

@mjesun mjesun deleted the mjesun:fix-worker-queues branch Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.