Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

quick fix kill command under windows #3106

Merged
merged 4 commits into from
Nov 27, 2020
Merged

Conversation

J-shang
Copy link
Contributor

@J-shang J-shang commented Nov 20, 2020

No description provided.

@J-shang J-shang mentioned this pull request Nov 20, 2020
9 tasks
@liuzhe-lz liuzhe-lz self-requested a review November 23, 2020 02:35
process.on('SIGTERM', cleanUp);
process.on('SIGBREAK', cleanUp);
process.on('SIGINT', cleanUp);
//not sure if 'SIGKILL' signal can be listened
Copy link
Contributor

Choose a reason for hiding this comment

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

It can't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix it

@SparkSnail
Copy link
Contributor

@liuzhe-lz So change back to CTRL_BREAK_EVENT is ok?

@@ -201,7 +188,11 @@ process.on(getStopSignal(), async () => {
hasError = true;
log.error(`${err.stack}`);
} finally {
await log.close();
log.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove await?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logger.close() is a sync function

@liuzhe-lz
Copy link
Contributor

@liuzhe-lz So change back to CTRL_BREAK_EVENT is ok?

On Windows SIGTERM does not trigger handler. It acts like SIGKILL in fact.
Seems the pipeline no longer fails after some random change. Feature is more important than pipeline anyway. I will disable the pipeline for this release if the problem come again.

@liuzhe-lz liuzhe-lz merged commit 62d5812 into microsoft:master Nov 27, 2020
@J-shang J-shang deleted the fix-killsig branch December 15, 2020 02:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants