Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: close compiler on SIGINT #2919

Closed
wants to merge 1 commit into from
Closed

Conversation

snitin315
Copy link
Member

What kind of change does this PR introduce?
fix

Did you add tests for your changes?
No
If relevant, did you update the documentation?
No
Summary

Fix #2918

Does this PR introduce a breaking change?
None

Other information
No

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Also let's add test on 130 code and close for serve (it is already implemented in dev server)

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #2919 (52d9a0c) into master (06cd267) will decrease coverage by 0.12%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2919      +/-   ##
==========================================
- Coverage   95.13%   95.00%   -0.13%     
==========================================
  Files          31       31              
  Lines        1684     1701      +17     
  Branches      481      484       +3     
==========================================
+ Hits         1602     1616      +14     
- Misses         82       85       +3     
Impacted Files Coverage Δ
packages/webpack-cli/lib/webpack-cli.js 96.34% <77.77%> (-0.18%) ⬇️
packages/serve/src/index.ts 84.52% <94.44%> (+0.24%) ⬆️

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 06cd267...52d9a0c. Read the comment docs.

@snitin315 snitin315 force-pushed the sigint-close-compiler branch from af33801 to 859a6c5 Compare August 25, 2021 14:05
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, let's add tests for build/watch/serve, just enable logging and check on https://github.com/webpack/webpack/blob/main/lib/cache/PackFileCacheStrategy.js#L1263 or you can implement custom plugin and use compiler.cache.hooks.shutdown.tapPromise() with { stage: 9999 } to ensure we will run after cache was stored

@snitin315 snitin315 force-pushed the sigint-close-compiler branch from b3c5f1f to 5da505a Compare August 29, 2021 09:39
@snitin315 snitin315 force-pushed the sigint-close-compiler branch from 5da505a to 52d9a0c Compare August 29, 2021 10:02
@alexander-akait
Copy link
Member

I will add tests

@alexander-akait
Copy link
Member

@snitin315 We need improve logic here: we need allow to graceful close and allow immutability disable, my idea - when developer run CTRL + C we try to close(), if CTRL + C pressed again we close immediately, also we need handle const signals = ["SIGINT", "SIGTERM"]; signals, for serve we need disable setupExitSignals and move this logic here https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L1088 (in future major release we will remove this option, because it is wrong place for this, developer should handle closing manually)

@snitin315
Copy link
Member Author

Makes sense, I will update it in near future.

@alexander-akait
Copy link
Member

@snitin315 hello, let's finish, just copy logic from dev server

@snitin315
Copy link
Member Author

@alexander-akait sure 👍🏻

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Could you rebase?

@@ -132,6 +132,40 @@ class ServeCommand {

const servers = [];

const stopAllServers = () => {
Promise.all(
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to make this a variable and passing the variable to the promise expression

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filesystem cache corruption on SIGINT
4 participants