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

Jump to bottom

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
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.13

Comments

@elgohr
Copy link

elgohr commented Nov 11, 2018 •
edited

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

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

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

@odeke-em odeke-em added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 11, 2018
@neelance
Copy link
Member

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 WebAssembly issues label Nov 11, 2018
@agnivade agnivade added this to the Go1.13 milestone Nov 12, 2018
@agnivade
Copy link
Contributor

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

@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

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 commented Nov 19, 2018 •
edited

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

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

This was referenced Nov 21, 2018
Closed
Closed
bradfitz pushed a commit that referenced this issue Nov 21, 2018
@neelance
syscall: add dummy SIGTERM constant to js/wasm
9ef1455 
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.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
6 participants
@neelance @agnivade @odeke-em @gopherbot @nlepage @elgohr