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

syscall: define SIGTERM for WASM #28719

Closed
elgohr opened this issue Nov 11, 2018 · 7 comments
Closed

syscall: define SIGTERM for WASM #28719

elgohr opened this issue Nov 11, 2018 · 7 comments

Comments

@elgohr
Copy link

@elgohr elgohr commented Nov 11, 2018

What version of Go are you using (go version)?

$ go version
go version go1.11 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

I tried to use syscall.SIGTERM in GOOS=js GOARCH=wasm.
Various libraries, for example see onsi/ginkgo#532 , are listening on syscall.SIGTERM.
As syscall.SIGTERM is not defined in WASM right now, $ GOOS=js GOARCH=wasm go test results in an error.
spec_runner.go:220:33: undefined: syscall.SIGTERM

On the other hand, running $ go test results in an error, as syscall/js is only available when using GOOS=js GOARCH=wasm

What did you expect to see?

I would expect syscall.SIGTERM to be defined as window.unload (https://developer.mozilla.org/de/docs/Web/Events/unload)

What did you see instead?

github.com/onsi/ginkgo/internal/specrunner
../../../../../pkg/mod/github.com/onsi/ginkgo@v1.6.0/internal/specrunner/spec_runner.go:220:33: undefined: syscall.SIGTERM
../../../../../pkg/mod/github.com/onsi/ginkgo@v1.6.0/internal/specrunner/spec_runner.go:245:33: undefined: syscall.SIGTERM
@odeke-em odeke-em changed the title syscall.SIGTERM in WASM syscall: define SIGTERM for WASM Nov 11, 2018
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Nov 11, 2018

Thank you for filing this issue and welcome to the Go project @elgohr!

I'll kindly /cc some experts @ianlancetaylor @neelance

@neelance
Copy link
Member

@neelance neelance commented Nov 11, 2018

Thanks for filing this.

One one hand, there is the Go proverb "Syscall must always be guarded with build tags." (https://go-proverbs.github.io/), so code shouldn't expect syscall.SIGTERM to exist.

On the other hand, the signal SIGTERM is very common and there are already some other signals defined for wasm/js because of stdlib dependencies.

I would be okay with adding a dummy for SIGTERM. wasm/js currently does not support signals at all, but it might help with compatibility like in the case mentioned above.

Linking it to window.unload is probably not a good idea, since the semantics are different. SIGTERM means "please exit", window.unload means "exiting now".

@neelance neelance added the Arch-Wasm label Nov 11, 2018
@agnivade agnivade added this to the Go1.13 milestone Nov 12, 2018
@agnivade
Copy link
Contributor

@agnivade agnivade commented Nov 15, 2018

Some more thoughts - should SIGTERM close the window ? Or rather exit from the WASM session ? The signal is sent to the wasm binary, not the browser tab, so I think it is a bit unexpected to close the entire browser tab.

A webapp may have multiple wasm applications running, so sending SIGTERM to one should not close all of them IMO.

@neelance
Copy link
Member

@neelance neelance commented Nov 15, 2018

@agnivade As mentioned above, wasm/js (or WebAssembly itself) has no real support for signals at all, so I don't see how what you wrote fits in.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Nov 15, 2018

Ah ! Sorry about that. Adding a dummy makes sense to me. We should not go about taking any action until the wasm spec properly supports signals.

@nlepage
Copy link

@nlepage nlepage commented Nov 19, 2018

Linking it to window.unload is probably not a good idea, since the semantics are different. SIGTERM means "please exit", window.unload means "exiting now".

There is the beforeunload event which would be a better fit, it is fired "when the window, the document and its resources are about to be unloaded".

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 20, 2018

Change https://golang.org/cl/150477 mentions this issue: syscall: add dummy SIGTERM constant to js/wasm

@gopherbot gopherbot closed this in b44cd15 Nov 20, 2018
This was referenced Nov 21, 2018
bradfitz pushed a commit that referenced this issue Nov 21, 2018
The js/wasm architecture does not support signals at all, but there are
already some signal constants defined because of stdlib dependencies.
This change adds a dummy constant for syscall.SIGTERM as well, to make
js/wasm compatible with more existing Go code.

There is the Go proverb "Syscall must always be guarded with build
tags.", so code should not expect syscall.SIGTERM to exist. Still,
adding SIGTERM should do more good than harm.

Fixes #28719.

Change-Id: I3554b484f96a21427491c04eb1dd57e7af5bd62f
Reviewed-on: https://go-review.googlesource.com/c/150477
Run-TryBot: Richard Musiol <neelance@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.