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

Interrupt signal propagation #1496

Closed

Conversation

hanjukim
Copy link

@hanjukim hanjukim commented Jul 8, 2020

Description

If we embed wasmer to a process, wasmer will override interrupt(SIGINT) signal handler. In result, previous signal handler will not be called when SIGINT is fired, and if it gets fired again, process will be stopped by process::abort() which leads to output a long stack trace.

Related Issue: #842

Review

  • Add a short description of the the change to the CHANGELOG.md file

@hanjukim hanjukim requested a review from Hywan as a code owner July 8, 2020 08:47
@hanjukim hanjukim marked this pull request as draft July 10, 2020 05:42
@hanjukim hanjukim marked this pull request as ready for review July 10, 2020 05:43
@hanjukim
Copy link
Author

hanjukim commented Jul 15, 2020

@Hywan Could you review this PR? This change is essential to shutdown process gracefully.

@syrusakbary
Copy link
Member

Hi @hanjukim we are in the mid of the process of upgrading Wasmer, that's why we are a bit slower replying here.

We will be super happy to invite you to it so you can try the new Wasmer, would you mind joining our slack channel so I can follow up there and make sure the issue is fixed? https://slack.wasmer.io/ Thanks! 🤗

@Hywan
Copy link
Contributor

Hywan commented Jul 16, 2020

@hanjukim Sorry for the late reply. As @syrusakbary told you, a new version of Wasmer is coming, and your issue is likely to be resolved already. If you accept the invitation, you can try to replicate your issue. Thoughts?

@hanjukim
Copy link
Author

@hanjukim Sorry for the late reply. As @syrusakbary told you, a new version of Wasmer is coming, and your issue is likely to be resolved already. If you accept the invitation, you can try to replicate your issue. Thoughts?

@syrusakbary I justed joined to wasmer slack. Which branch should I use for testing new version candidate? Thanka for your help.

@hanjukim
Copy link
Author

hanjukim commented Jul 27, 2020

@Hywan @syrusakbary
Ok. I reviewd new Wasmer (wasmer-reborn) and it seems a lot of things are changing. We are using cosmwasm which is a library that provides building cosmos-compatible wasm smart contracts and it is dependent on wasmer. Is there any chance that this PR could be merged to next release of (old) wasmer? If not, we will have to decide to fork every dependent repositories for our software release.

@syrusakbary
Copy link
Member

I reviewd new Wasmer (wasmer-reborn) and it seems a lot of things are changing

That's right. Thanks though for taking a sneak peek of the project!

Is there any chance that this PR could be merged to next release of (old) wasmer?

Yeah, I think we can do that to facilitate your work!

@nlewycky
Copy link
Contributor

While reviewing this PR, I began to wonder why we were installing a hander for SIGINT at all, and discovered that it's tied to the managed cfg feature. I moved the relevant code to be conditional under that feature, so we no longer install any handler for SIGINT by default. #1552

If you'd like to update this PR so that you can use managed and install your own signal handler for sigint, please rebase on top of the 0.x branch. Thanks!

@syrusakbary
Copy link
Member

This PR was done for the old codebase, which as commented by Nick it's also solved in master. Closing PR.

PS: thanks anyway for the contribution @hanjukim !

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.

4 participants