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

Bugfix: Remove close-handler for powershell child process #865

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evekto
Copy link

@evekto evekto commented Nov 6, 2023

This PR tries to fix a race condition which occurs when calling powerShellRelease and powerShellStart in sequence.

Background: We need to restart the powershell instance from time to time in a long running process as its memory consumption grows without limit.

Currently, 2 problems can arise in the close-event handler if powerShellStart() is called after powerShellRelease():

  1. The variable '_psChild' could be null if the close-event was fired after the null assignment but before the spawn() call returns.
    (in my tests when powerShellStart is called 1s after powerShellRelease).

  2. The signal from the kill()-call could be delivered to the newly created powershell subprocess.
    (in my tests when powerShellStart is called directly after powerShellRelease).

My fix removes the problematic lines, because I don't see a reason for calling 'kill' on the subprocess in its 'close'-event handler.
According to the NodeJS documentation, this handler is called 'after a process has ended and the stdio streams of a child process have been closed'1. Maybe I'm missing something here, in that case we could solve the referencing issue using a local variable for the result of 'spawn', use that in the handler and assign the local variable to the global one after setting the handlers.

2 problems could arise in the close-event handler if powerShellStart() is called after powerShellRelease():

1. the variable _psChild could be null if the close-event was fired after the null assignment but before the spawn() call returns.
(in my tests when powerShellStart is called 1s after powerShellRelease).

2. the signal from the kill()-call could be delivered to the newly created powershell subprocess.
(in my tests when powerShellStart is called directly after powerShellRelease).

In my view, the close handler is not necessary.
@evekto evekto force-pushed the bugfix/powershell-restart-race-condition branch from 4f5500c to 8aa1ebe Compare April 12, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant