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.

Sign up for GitHub

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

Jump to bottom

Run Python process as child process of NodeJS. #146

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Run Python process as child process of NodeJS. #146

wants to merge 6 commits into from

Conversation

joeriexelmans
Copy link

@joeriexelmans joeriexelmans commented Oct 7, 2022 •
edited
Loading

Motivation

For the NodeJS process to "work", it depends on the Python process to also be running. The Python process has no dependencies. Therefore, it makes sense to launch the Python script as a child process of the NodeJS script.

To be more precise, the correct order (of a normal scenario) should be:

Startup:

  1. NodeJS script is started
  2. Python script is started
  3. Python server starts listening
  4. NodeJS server starts listening

Shutdown (graceful):

  1. NodeJS server stops listening
  2. Python server stops listening
  3. Python script exits
  4. NodeJS script exits

Features implemented

  • By default, the Python server is launched as a child process of the NodeJS server.
  • Python's stdout and stderr are piped and interleaved with NodeJS's stdout and stderr. To distinguish between the two, Python's output has a different color (yellow/red for stdout/stderr).
  • Graceful shutdown of both processes in the correct order when SIGINT or SIGTERM is sent to NodeJS server.
  • Forced shutdown of NodeJS is triggered by 2x SIGINT (Ctrl+C in terminal), or when an uncaught JavaScript exception occurs. A forced shutdown will send a SIGKILL to the Python process.
  • Ability to run NodeJS server without the Python child process (flag --without-child)

I also deleted the bash script that starts both NodeJS & Python. I think it has become obsolete.

Limitations

  • When a SIGKILL is sent to the NodeJS process, the Python process will remain running. This is because there is no way for a Unix process to catch a SIGKILL.

Things to check

  • Not yet tested on Windows/Mac OS
  • It seems that when there is an active WebSocket connection, the HTTP server takes forever to shut down. I could not find the proper way to close all WebSocket connections. A forced shutdown works, of course.

Resolves #145

@BentleyJOakes
Copy link
Member

Hi Joeri. Thanks for the PR. It's very much appreciated.

Looking at the changes, and thinking through the implications of how AToMPM runs, I would prefer that this change be opt-in for now. That is, it is a child process only when the --child_mt_process flag is passed in.

My primary reasoning is that there's a lot that can go wrong with the MT server in one particular case: The standalone version on Windows, which must be well-supported. Many difficulties can arise with Python not being installed correctly, or the required packages not being there. I would prefer that the user tackle these issues when they need to run the MT server, in their own terminal window for clarity.

So I would be conservative and leave the status quo as is for now in most cases. But yes, for development on Linux, it would be nice to have a cleaner startup/shutdown.

Therefore I would (kindly) ask for the following changes in this PR:
a) Don't modify run_AToMPM_local.sh. Sometimes it's useful. Note that similar logic is also present in run_tests.sh.
b) Change the flag to be opt-in, --child_mt_process (or a better name)
c) Extract the Python child setup into its own function in httpwsd.js.
d) Add the usage of the flag into README.md in the Developing AToMPM section. Even better, split that section off into DEVELOPING.md as it's getting long, and point to that document in the README.
e) Do some (light) testing on Windows, just to see if the opt-in flag works there too.

Again, many thanks for this suggestion!

@joeriexelmans
Copy link
Author

Hi Bentley,
Took care of points (a-c).
For point (d), I documented the new flag --with-python in the Usage section, because I think it's primarily for "normal use" that one would want to use this flag, whereas for development, one would want to run the Python server in a separate terminal.
For point (e), I cannot test on Windows, because I don't have Windows. Given that the flag is now opt-in, it won't cause trouble for Windows users.
What do you think?
Joeri

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Launch python transformation engine as subprocess of nodejs?
2 participants
@joeriexelmans @BentleyJOakes