Fix worker call IDs with multiple worker instances #3481
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, 'currentCallMethodId' was a field on each instance of PrettierWorkerInstance, however, there was only a single instance of 'worker'. So when there were multiple instances of PrettierWorkerInstance, they would each have their own 'currentCallMethodId' which could overlap. This meant that the instances could not reliably get results back from the worker, as there were multiple results with the same ID. This results in many issues, such as overwriting files with the contents of other files while formatting. This change moves 'currentCallMethodId' outside PrettierWorkerInstance, so that all instances will use the same value and cannot have overlapping IDs.
mogzol
force-pushed
the
fix-worker-call-id
branch
from
0a539b7 to
ccc9e6a
Compare
The same issue that existed for the callMethod messages also existed for the import messages, where multiple PrettierWorkerInstances could get messages meant for a different instance, so the 'version' field could end up with the wrong value. This changes the code to include an ID in all messages sent to the worker, so that the PrettierWorkerInstances can properly determine whether a message is meant for them or not.
|
Noticed that the same issue existed for the "import" messages (which didn't use IDs at all), resulting in PrettierWorkerInstances having incorrect version numbers. I just pushed a change to fix that issue as well. |
|
I did not mean to close this, oops, reopening. @ntotten Any chance you could review this? I think it is a pretty serious issue for people working in monorepos with multiple packages that use prettier. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3423, #3040
Previously,
currentCallMethodIdwas a field on each instance of PrettierWorkerInstance, however, there is only a single instance ofworker. So when multiple instances of PrettierWorkerInstance sent messages to the worker, they would each use their owncurrentCallMethodId, resulting in multiple messages with the same ID. This meant that the instances could not reliably get results back from the worker, as there were multiple results with the same ID. This caused many issues, such as overwriting files with the contents of other files while formatting.Additionally, the "import" messages to the worker did not use IDs at all, so a PrettierWorkerInstance could end up setting the wrong version number.
This PR changes the call IDs to use a variable outside PrettierWorkerInstance, so that all instances will use the same value and cannot have overlapping IDs. It also ensures that all messages to and from the worker include an ID, so that the PrettierWorkerInstances can properly determine who a message is for.
CHANGELOG.mdwith a summary of your changes