Skip to content

feat: relaunch on SIGUSR2 #4979

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

Merged
merged 4 commits into from
Apr 27, 2022
Merged

feat: relaunch on SIGUSR2 #4979

merged 4 commits into from
Apr 27, 2022

Conversation

code-asher
Copy link
Member

This is because Node uses SIGUSR1 to enable the debug listener so even
if you just want to restart code-server you end up enabling the debug
listener as well.

Opted to leave the SIGUSR1 handler in to avoid breaking existing
workflows even though it does mean even if you only want to enable the
debug listener you will end up restarting code-server as well. We could
consider removing it after a transition phase.

This is because Node uses SIGUSR1 to enable the debug listener so even
if you just want to restart code-server you end up enabling the debug
listener as well.

Opted to leave the SIGUSR1 handler in to avoid breaking existing
workflows even though it does mean even if you only want to enable the
debug listener you will end up restarting code-server as well.  We could
consider removing it after a transition phase.
@code-asher code-asher requested a review from a team March 10, 2022 18:16
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Nice fix! What prompted this btw?

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #4979 (8e9691a) into main (fc75db6) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4979      +/-   ##
==========================================
- Coverage   71.75%   71.62%   -0.13%     
==========================================
  Files          30       30              
  Lines        1685     1688       +3     
  Branches      375      375              
==========================================
  Hits         1209     1209              
- Misses        407      410       +3     
  Partials       69       69              
Impacted Files Coverage Δ
src/node/wrapper.ts 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc75db6...8e9691a. Read the comment docs.

@code-asher
Copy link
Member Author

This has been mostly an internal thing so I have just lived with the debugger turning on but today I was asked how to restart code-server without rebuilding a workspace and it felt bad that I had to say "here is how...but the debugger will turn on" so I decided to fix it real swift. 😄

@code-asher
Copy link
Member Author

It has been on my mind for a very long time, should have done this ages ago.

@code-asher code-asher changed the title Relaunch on SIGUSR2 feat: relaunch on SIGUSR2 Mar 10, 2022
@jsjoeio
Copy link
Contributor

jsjoeio commented Mar 10, 2022

decrease coverage by 0.12%

😧

@code-asher
Copy link
Member Author

lol would probably be good to add a test for relaunching

@code-asher code-asher marked this pull request as draft March 29, 2022 16:40
@jsjoeio
Copy link
Contributor

jsjoeio commented Mar 29, 2022

bump @code-asher what should we do about this? We could merge and disregard the test coverage drop, up to you.

@code-asher
Copy link
Member Author

I was hoping to add tests although it will not actually increase coverage I guess because we have to spawn a separate process to test it.

@jsjoeio
Copy link
Contributor

jsjoeio commented Mar 30, 2022

Up to you then! I mean we can always add tests in another place to keep coverage high

@jsjoeio
Copy link
Contributor

jsjoeio commented Apr 26, 2022

@code-asher not to necrobump this, but what should we do with this?

@code-asher
Copy link
Member Author

Best options right now are probably to merge as-is or close; unlikely I can add tests right now 😢

@jsjoeio
Copy link
Contributor

jsjoeio commented Apr 27, 2022

I'm okay with merging as is if you are!

@code-asher code-asher marked this pull request as ready for review April 27, 2022 15:09
@code-asher
Copy link
Member Author

That sounds good to me!

@code-asher code-asher merged commit 4e93db5 into coder:main Apr 27, 2022
@code-asher code-asher deleted the sigusr2 branch April 27, 2022 15:10
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.

2 participants