fix: Fixing a potential race condition and possibly memory leak in "powerShellStart"
#696
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
…powerShellStart` could be called multiple times, before `powerShellRelease` is called. Imagine the following scenario: 1. function 1 calls `si.powerShellStart()` followed by `si.system(cb)` (or the promise version). It'll call "si.powerShellRelease" once it gets the response in the callback. 2. function 2 makes similar calls `si.powerShellStart()` followed by `si.os(cb)`. There are a couple of issues here: 1. There's a chance that the call by function 2 could end up creating a new "PowerShell" process, which is not desirable, since creating "PowerShell" processes are expensive operation. It is also unnecessary to create the second "PowerShell" process, since function 2 should be able to just "reuse" the same "PowerShell" created by function 1. 2. The "PowerShell" process created by function 1 now becomes an "orphan", because `_psChild` is global therefore it's overwritten by function 2. The callback of function 1 may never be able to release it, where the "PowerShell" process created by function 2 are released twice. This commit changes `powerShellStart` function so that it first check if a `persisted` "PowerShell" process already existed, and just reuse it if one is available.
|
Hi @sebhildebrandt, would be very much appreciated if you could have a look at this as soon as possible. |
whoisyulu
changed the title
race condition and possibly memory leak in "powerShellStart"race condition and possibly memory leak in **powerShellStart**
whoisyulu
changed the title
race condition and possibly memory leak in **powerShellStart**race condition and possibly memory leak in "powerShellStart"
|
hmmmm, I think this change is not correct, because any caller (function 1 or function 2 in above example) could be killing the process before both of them are finished with it. We'll probably need a way to track if I'll close this PR for now. |
|
Perhaps this https://github.com/Shopify/quilt/tree/main/packages/semaphore could help. |
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.
We've been use
powerShellStartandpowerShellReleasein our project to minimise the number "PowerShell" processes created by SystemInformation in order to work around #626.However, we've noticed a couple of potential problems, when
powerShellStartare called multiple times, beforepowerShellReleaseis called.Imagine the following scenario:
si.powerShellStart()followed bysi.system(cb)(or the promise version). It'll callsi.powerShellReleaseonce it gets the response in the callback.si.powerShellStart()followed bysi.osInfo(cb).(In reality, function 1 & 2 are in two different "context" therefore have no knowledge of each other regarding when which one will make the call first, hence very tricky to synchronise them.)
There are a couple of issues here:
_psChildis global therefore it's overwritten by the one created by function 2. The callback of function 1 may never be able to release it (hence possible memory leak), where the "PowerShell" process created by function 2 are released twice.This commit changes
powerShellStartfunction so that it first checks if apersisted"PowerShell" process already existed, and just reuse it if one is available.