-
Notifications
You must be signed in to change notification settings - Fork 11k
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: decoder error preventing succesfull app subprocess restart #34858
Conversation
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: db60bc1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #34858 +/- ##
========================================
Coverage 59.19% 59.19%
========================================
Files 2821 2821
Lines 67633 67633
Branches 15048 15048
========================================
Hits 40033 40033
Misses 24784 24784
Partials 2816 2816
Flags with carried forward coverage won't be shown. Click here to find out more. |
f43b821
to
fcce702
Compare
/patch |
after that just run |
/patch |
after that just run |
/patch |
/bark |
AU AU |
/patch |
Pull request #34879 added to Project: "Patch 7.1.1" |
/backport 7.0.4 |
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands:
after that just run |
/backport 7.0.4 |
Pull request #34880 added to Project: "Patch 7.0.4" |
Proposed changes (including videos or screenshots)
If msgpack's
decodeStream
throws an error, the internal state of the decoder becomes "invalid" (the encoded malformed message stays cached) and the engine was not capable of reestabilishing communications with any other subprocesses as the decoder instance never disposes of the malformed data. This essentially rendered the whole apps-engine incapable of communication, as the decoder instance was shared among apps.To solve this, now the engine will instantiate a new decoder every time it spawns a subprocess. This guarantees that the internal state of the decoder for a subprocess will never impact the communication of other subprocesses, and also allows the engine to reestablish communication with a new subprocess of an app after a
Decode error
.Another side effect of this problem could be observed on the subprocess side. Since the engine stopped reading data from the subprocess'
stdout
pipe, the operation system was blocking thewrite
syscall that the subprocess made and essentially made it stuck with an ever growing queue of ougoing messages.Note: I've also tried reestablishing the communication over the same pipe that we had with the subprocess - since the state of the subprocess wasn't invalid, it made sense to try that. However, the msgpack decoder left the
stdout
stream of the ChildProcess in an unrecoverable error state, so I was left with restarting the whole subprocess.Issue(s)
CONN-448
Steps to test or reproduce
Further comments