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/js: unclear behavior of js-triggered and wrapped functions #34324

Open
torbenschinke opened this issue Sep 16, 2019 · 9 comments · May be fixed by #34551
Open

syscall/js: unclear behavior of js-triggered and wrapped functions #34324

torbenschinke opened this issue Sep 16, 2019 · 9 comments · May be fixed by #34551

Comments

@torbenschinke
Copy link

@torbenschinke torbenschinke commented Sep 16, 2019

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

go version go1.13 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/tschinke/Library/Caches/go-build"
GOENV="/Users/tschinke/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/tschinke/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/tschinke/repos/wdy_ausbildung/trainiety/app/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bv/6rt5ck194ls704dz9p4c0hlh0000gn/T/go-build951955169=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

<div id="outer" class="fullscreen">
  <div id="inner" class="dialog">
  </div>
</div>

var outerDiv js.Value = ...
var innerDiv js.Value = ...

outerDiv.Call("addEventListener", "click", js.FuncOf(func(this js.Value, args []js.Value) interface{} {
		log.Println("listener called: ", this, this.Get("id"))
		return nil
	}), once)
[...]
innerDiv.Call("addEventListener", "click", js.FuncOf(func(this js.Value, args []js.Value) interface{} {
                 args[0].Call("stopPropagation")
		log.Println("listener called: ", this, this.Get("id"))
		return nil
	}), once)

What did you expect to see?

Naturally I expected that the listener to the inner div should always be called first. However looking at the documentation of js.FuncOf, I don't know what to expect:

Invoking the JavaScript function will synchronously call the Go function fn with the value of JavaScript's "this" keyword and the arguments of the invocation.
[...]
A wrapped function triggered by JavaScript's event loop gets executed on an extra goroutine.
Blocking operations in the wrapped function will block the event loop.
[...]
A blocking function should therefore explicitly start a new goroutine.

The logic of these statements is contradictory:

  • What exactly is meant by a "synchronous call" in the first statement?
  • If a wrapped and triggered function is always executed in a goroutine, why should it block the event loop?

What did you see instead?

I tried to use stopPropagation but the calling order is always wrong (outer-div first), independent of registration order. Also if a callback is always executed within a new goroutine, it sounds like stopPropagation et. al cannot be used properly.

The documentation of js.FuncOf does not help me much to understand the actual calling behavior or at least if this works as "expected".

@toothrot toothrot changed the title wasm: unclear behavior of js-triggered and wrapped functions syscall/js: unclear behavior of js-triggered and wrapped functions Sep 17, 2019
@toothrot

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Sep 17, 2019

For asking questions about the language, see one of our forums: https://golang.org/wiki/Questions It is not completely clear to me whether this is a documentation bug or a question about correct usage.

/cc @bradfitz @neelance who may be able to better assist.

@neelance

This comment has been minimized.

Copy link
Member

@neelance neelance commented Sep 17, 2019

This works fine for me. The code I tested:

package main

import (
	"fmt"
	"syscall/js"
)

func main() {
	outerDiv := js.Global().Get("document").Call("getElementById", "outer")
	innerDiv := js.Global().Get("document").Call("getElementById", "inner")

	outerDiv.Call("addEventListener", "click", js.FuncOf(func(this js.Value, args []js.Value) interface{} {
		fmt.Println("outer listener called")
		return nil
	}))
	innerDiv.Call("addEventListener", "click", js.FuncOf(func(this js.Value, args []js.Value) interface{} {
		args[0].Call("stopPropagation")
		fmt.Println("inner listener called")
		return nil
	}))

	select {}
}
@torbenschinke

This comment has been minimized.

Copy link
Author

@torbenschinke torbenschinke commented Sep 18, 2019

@neelance thx for the example. I found my fault: I passed a wrong additional parameter to addEventListener which evaluated to true and is then interpreted as the useCapture flag, which caused a "wrong" event bubbling, as specified in https://developer.mozilla.org/de/docs/Web/API/EventTarget/addEventListener

However regarding the documentation of js.Func, I propose a small change like the following to be a bit more explicit:

A wrapped function triggered by JavaScript's event loop gets executed on an extra goroutine, however the event loop waits until the goroutine returns. Therefore blocking operations in the wrapped function should explicitly start a new goroutine.

Otherwise this ticket can be closed.

@neelance

This comment has been minimized.

Copy link
Member

@neelance neelance commented Sep 19, 2019

Current doc:

A wrapped function triggered by JavaScript's event loop gets executed on an extra goroutine. Blocking operations in the wrapped function will block the event loop. As a consequence, if one wrapped function blocks, other wrapped funcs will not be processed. A blocking function should therefore explicitly start a new goroutine.

Proposed doc:

A wrapped function triggered by JavaScript's event loop gets executed on an extra goroutine, however the event loop waits until the goroutine returns. Therefore blocking operations in the wrapped function should explicitly start a new goroutine.

@torbenschinke Could you please give me some more detail on the reasons for your change? Doing any blocking operation directly in the wrapped function most likely results in a "deadlock" error. For this result, "the event loop waits until the goroutine returns" sounds a bit too innocent. :)

@torbenschinke

This comment has been minimized.

Copy link
Author

@torbenschinke torbenschinke commented Sep 22, 2019

@neelance The facts of the current documentation are not particularly coupled with each other. Each fact is correct, but because there is no specific logical glue between them, it is difficult to me to create a sufficient mental model, which is needed to understand a specific behavior.

For example, the statement that each wrapped function, which is called by JavaScript's event loop, gets executed on an extra goroutine is clear, but as long as the context is not described further, it makes it hard to understand consequences: for a gopher it is a surprise that a function can get executed on a goroutine which will block the callee, without using channels, waitgroups or mutexes.

Next, the statement about blocking operations is probably also correct. But what exactly is a blocking operation? Is it only related to epoll-like I/O operations or even a tight for-loop or is it calling syscall/js APIs?

Without these information, it seems very hard to anticipate the actual behavior and (expected) side effects of doing I/O (like causing deadlocks) or synchronous calls into Javascript (like stopping propagation).

@neelance

This comment has been minimized.

Copy link
Member

@neelance neelance commented Sep 22, 2019

I agree that we should try to make this as easily understandable as possible.

The statement about waiting for the function to return is actually a bit earlier in the documentation:

Invoking the JavaScript function will synchronously call the Go function fn with the value of JavaScript's "this" keyword and the arguments of the invocation.

Maybe this "synchronously" is not enough. Do you have any suggestion on how we can amend this part?

@torbenschinke

This comment has been minimized.

Copy link
Author

@torbenschinke torbenschinke commented Sep 23, 2019

I looked through some of your CLs and the js-Transport-RoundTripper and it looks quite complex in detail, so please correct me, if I got something wrong. But here is my attempt:

// add the intent: why do we want to use FuncOf?
FuncOf registers a function to be used by JavaScript.

// perhaps better at the beginning of the documentation, not that readers gave up reading to early
Func.Release must be called to free up resources when the function
will not be used any more.

// explain the in and out arguments
The Go function fn is called with the value of JavaScript's "this" keyword and the
arguments of the invocation. The return value of the invocation is
the result of the Go function mapped back to JavaScript according to ValueOf.

// now we can focus on explaining the lifecycle
Invoking the wrapped Go function from JavaScript will
pause the event loop and spawns a new go routine.
Other wrapped functions which are triggered during a call from Go to JavaScript
gets executed on the same goroutine.

// explain the consequences of that lifecycle
As a consequence, if one wrapped function blocks, JavaScript's event loop
is blocked until that func returns.
Hence, calling any async JavaScript API, which requires the event loop,
like fetch (http.Client), will cause an immediate deadlock.
Therefore a blocking function should explicitly start a new goroutine.

@neelance

This comment has been minimized.

Copy link
Member

@neelance neelance commented Sep 23, 2019

This is great, I like it a lot! Would you mind opening a CL or pull request for the change?

torbenschinke pushed a commit to torbenschinke/go that referenced this issue Sep 26, 2019
The existing documentation is improved to be more
explicit about the lifecycle and its consequences.

Fixes golang#34324
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 26, 2019

Change https://golang.org/cl/197458 mentions this issue: syscall/js: improve documentation of js.FuncOf

torbenschinke pushed a commit to torbenschinke/go that referenced this issue Nov 7, 2019
The existing documentation is improved to be more
explicit about the lifecycle and its consequences.

Fixes golang#34324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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