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

Issue 4030: Using SIGKILL instead of SIGTERM to terminate the process on timeout #4036

Merged
merged 1 commit into from Apr 9, 2019

Conversation

@jumde
Copy link
Contributor

jumde commented Apr 5, 2019

fix #4030

PR Builder passed here: https://staging.ci.brave.com/job/brave-browser-build-pr/job/PR-4036/4/

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Requested a security/privacy review as needed.

Test Plan:

  1. Verify that npm run test-security works on build machines.
  2. Remove any entry from lib/whitelistedUrlPrefixes.js and confirm it fails.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions.
@jumde jumde force-pushed the test-security-killsignal branch from b217a99 to bf5ee58 Apr 7, 2019
@jumde jumde self-assigned this Apr 7, 2019
@jumde jumde added the QA/No label Apr 7, 2019
@jumde jumde force-pushed the test-security-killsignal branch from bf5ee58 to 79704da Apr 8, 2019
@jumde jumde changed the title WIP - Issue 4030: Using SIGKILL instead of SIGTERM to terminate the process on timeout Issue 4030: Using SIGKILL instead of SIGTERM to terminate the process on timeout Apr 8, 2019
@mihaiplesa
Copy link
Collaborator

mihaiplesa commented Apr 8, 2019

We could try SIGTERM first and then SIGKILL (1m after), what do you all think?

@diracdeltas
Copy link
Member

diracdeltas commented Apr 8, 2019

We could try SIGTERM first and then SIGKILL (1m after), what do you all think?

that sgtm. or add an env variable for the build machines which need SIGKILL

@jumde
Copy link
Contributor Author

jumde commented Apr 8, 2019

We could try SIGTERM first and then SIGKILL (1m after), what do you all think?

that sgtm. or add an env variable for the build machines which need SIGKILL

I'll add an environment variable.

@mihaiplesa: let me know if BUILD_MACHINE=yes sounds good as the env variable and value?

EDIT: Updated @mihaiplesa @diracdeltas - let me know if the code looks good now

@jumde jumde force-pushed the test-security-killsignal branch from 79704da to 1615616 Apr 8, 2019
@mihaiplesa
Copy link
Collaborator

mihaiplesa commented Apr 9, 2019

We have a var named RELEASE_TYPE, if that is set it's an indication you're in a build machine (it gets values of release and ci) so we should use this instead.

@mihaiplesa mihaiplesa force-pushed the test-security-killsignal branch from eab91c1 to ce628a9 Apr 9, 2019
lib/start.js Outdated Show resolved Hide resolved
@mihaiplesa mihaiplesa force-pushed the test-security-killsignal branch from ce628a9 to 7ef06af Apr 9, 2019
Copy link
Collaborator

mihaiplesa left a comment

have tested and works as expected

@mihaiplesa mihaiplesa requested a review from diracdeltas Apr 9, 2019
@bsclifton bsclifton dismissed diracdeltas’s stale review Apr 9, 2019

change she requested as made with 7ef06af

@mihaiplesa mihaiplesa self-requested a review Apr 9, 2019
@bsclifton bsclifton merged commit 7fd4b80 into master Apr 9, 2019
1 check was pending
1 check was pending
continuous-integration/jenkins/pr-head This commit is being built
Details
@bsclifton bsclifton deleted the test-security-killsignal branch Apr 9, 2019
@bsclifton bsclifton added this to the 0.65.x - Nightly milestone Apr 9, 2019
bsclifton added a commit that referenced this pull request Apr 9, 2019
Issue 4030: Using SIGKILL instead of SIGTERM to terminate the process on timeout
bsclifton added a commit that referenced this pull request Apr 9, 2019
Issue 4030: Using SIGKILL instead of SIGTERM to terminate the process on timeout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.