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

Closing RPE app in Windows leaves the backend server running in some instances causing clashes on new rpe instance #263

Conversation

shivaahir158
Copy link
Collaborator

@shivaahir158 shivaahir158 commented Nov 12, 2024

Motivate of the pull request

  • To address an existing issue. If so, please provide a link to the issue:
  • Breaking new feature. If so, please describe details in the description part.

Describe the technical details -

  1. Added signal handlers in restapi_server.py to handle SIGINT and SIGTERM shutdowns for the Flask server.
  2. Implemented a server_shutdown function to log and execute the backend shutdown process cleanly upon receiving a shutdown signal.
  3. Created a Playwright test to launch and close the Electron app 10 times (can do 100 times as well, but job run would be time consuming on CI, while testing I do an iteration of 100), checks that no lingering processes remain.
  4. Commented out dsp.test.js and clocking.test.js for now, I will uncomment them again when we shall have proper testing suites, for now doing e2e for app launch, restart, and checking if the app process is killed.
  5. Backend PR Closing RPE app in Windows leaves the backend server running in some instances causing clashes on new rpe instance #263

What is currently done? (Provide issue link if applicable)

What does this pull request change?

Which part of the code base require a change

  • Frontend:
  • Backend: restapi_server.py
  • Library: <Specify the library name, e.g. npm packages>
  • Plug-in: <Specify the plugin name, e.g. Webpack, jtest>
  • Documentation
  • Playwright tests restart.test.js, dsp.test.js, clocking.test.js
  • Continuous Integration (CI) scripts

Impact of the pull request

  • Require a change on Quality of Results (QoR)
  • Break backward-compatibility. If so, please list who may be influenced.

User can verify if the process has shut down, in the log file, fyi @NadeemYaseen :

image

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 50.63291% with 39 lines in your changes missing coverage. Please review.

Project coverage is 68.76%. Comparing base (f5dd78a) to head (9aef427).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/utils/serverAPI.js 10.71% 25 Missing ⚠️
backend/api/shutdown.py 50.00% 11 Missing ⚠️
backend/restapi_server.py 89.65% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #263      +/-   ##
==========================================
- Coverage   68.83%   68.76%   -0.07%     
==========================================
  Files         110      111       +1     
  Lines        9022     9052      +30     
  Branches      402      398       -4     
==========================================
+ Hits         6210     6225      +15     
- Misses       2812     2827      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@shivaahir158 shivaahir158 self-assigned this Nov 12, 2024
@shivaahir158 shivaahir158 marked this pull request as ready for review November 12, 2024 21:36
@shivaahir158 shivaahir158 added the enhancement New feature or request label Nov 12, 2024
Copy link
Contributor

@ravikiranchollangi ravikiranchollangi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not comment out the e2e tests. You can re-factor later.
How long does the 10times re-launch test case take?

@shivaahir158
Copy link
Collaborator Author

shivaahir158 commented Nov 13, 2024

Let's not comment out the e2e tests. You can re-factor later. How long does the 10times re-launch test case take?

@ravikiranchollangi , 10 iterations takes around 45s, also, I will push all the e2e test cases in the next PR, it needed some fixes which is been done, requesting to for your approval to this PR so we have a solution for Shutdown issue, and I shall raise one more PR for the e2e test cases.

@ravikiranchollangi ravikiranchollangi changed the title Shutdown issue Closing RPE app in Windows leaves the backend server running in some instances causing clashes on new rpe instance Nov 13, 2024
@shivaahir158
Copy link
Collaborator Author

Let's not comment out the e2e tests. You can re-factor later. How long does the 10times re-launch test case take?

@ravikiranchollangi , 10 iterations takes around 45s, also, I will push all the e2e test cases in the next PR, it needed some fixes which is been done, requesting to for your approval to this PR so we have a solution for Shutdown issue, and I shall raise one more PR for the e2e test cases.

@ravikiranchollangi SIGTERM is not a forced shutdown, to my knowledge, it’s a polite request for the process to terminate. When a process receives SIGTERM, it has the opportunity to perform any cleanup tasks before it exits, such as closing files, saving state, or logging. This makes SIGTERM a preferred way to smoothly (not forcibly) stop a process. SIGKILL is a forced shutdown.

I believe SIGTERM is not essential if SIGINT is the only signal sent by the frontend, but keeping both signals allows our backend to smoothly handle termination requests.

@NadeemYaseen
Copy link
Contributor

Adding a screenshot confirms that the backend honors the kill signal sent by the front end.

@shivaahir158
Copy link
Collaborator Author

shivaahir158 commented Nov 14, 2024

@NadeemYaseen , check the lastest changes, as discussed, I have created shutdown.py API, called the same api in restapi_server.py, and in cleanup.js I have called the shutdown api, please check the changes once, so we can discuss further. the push will fail because of yml changes that we did previously.

@shivaahir158
Copy link
Collaborator Author

@NadeemYaseen , fetch the latest changes, just removed the need to have axios and called the shutdown function in serverapi.js, and after that called n cleanup.js.

@NadeemYaseen
Copy link
Contributor

@NadeemYaseen , fetch the latest changes, just removed the need to have axios and called the shutdown function in serverapi.js, and after that called n cleanup.js.

@shivaahir158 I am facing the below issue, am I missing something,
image

@shivaahir158
Copy link
Collaborator Author

@NadeemYaseen , fixed. check now.

@shivaahir158 shivaahir158 deleted the task/RPE-154/Find-a-way-to-send-shut-down-signal-to-backend branch November 20, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants