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

runtime: js-wasm builder broken due to timeout #34768

Closed
katiehockman opened this issue Oct 8, 2019 · 11 comments

Comments

@katiehockman
Copy link
Contributor

commented Oct 8, 2019

The js-wasm builder is consistently failing after https://go-review.googlesource.com/c/go/+/199537 was merged. It's unclear exactly what exactly is going wrong from the test output, but this seems suspicious:

##### ../test/bench/go1

##### ../test

##### 
Test "test:4_10" ran over 20m0s limit (20m0.001274792s)

https://build.golang.org/log/0ebc31b62df29629efb94fc41ea07895f0b346bd

/cc @aclements @cherrymui @neelance

@neelance

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

It hangs here:

go/test/goprint.go

Lines 18 to 20 in ed7e430

for runtime.NumGoroutine() > 1 {
time.Sleep(10*time.Millisecond)
}

The additional goroutine that handles async events now also counts for NumGoroutine, so it never becomes 1.

@aclements @cherrymui I am not sure how to solve this the cleanest way. Do we need to adapt the test? Should the additional goroutine not count for NumGoroutine? Or should it not count as a user goroutine at all as defined by isSystemGoroutine?

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

In my opinion runtime.NumGoroutine should probably include the callback goroutine, as it is mostly running user code instead of runtime code. Then we should adjust the test. Unless there are a lot of code depending on the old behavior?

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

Related: could we start the callback goroutine lazily? Maybe at the first time a JS callback is created?

@dmitshur dmitshur added the Arch-Wasm label Oct 9, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@bcmills bcmills modified the milestones: Backlog, Go1.14 Oct 9, 2019
@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

If the fix for this test isn't trivial enough to apply right away, CL 199537 should be rolled back so that it does not mask other regressions on the builder. (We can roll it forward again when the fix for the test is ready.)

@neelance

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

I'll have a fix ready by tomorrow. :)

@gopherbot

This comment has been minimized.

Copy link

commented Oct 10, 2019

Change https://golang.org/cl/200477 mentions this issue: dashboard: re-enable js/wasm test dir tests

@gopherbot

This comment has been minimized.

Copy link

commented Oct 10, 2019

Change https://golang.org/cl/200438 mentions this issue: test: adjust a test to work with js/wasm's background goroutine

gopherbot pushed a commit to golang/build that referenced this issue Oct 10, 2019
They were removed to make the build faster but they've let a few
bugs slip by now.

Re-enable.

The 10 test shards adds 812 seconds of test time. Over 5 machines,
that's 162.4 seconds each. Add another test shard to bring it down
more.

js-wasm trybots are currently at ~350 seconds, so we have some budget
to get a bit slower and still be under 5 minutes.

Updates golang/go#34768

Change-Id: Ic40c226d26e873227510e644756b0cc969151186
Reviewed-on: https://go-review.googlesource.com/c/build/+/200477
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot gopherbot closed this in 6dc740f Oct 10, 2019
@gopherbot

This comment has been minimized.

Copy link

commented Oct 10, 2019

Change https://golang.org/cl/200497 mentions this issue: runtime: make goroutine for wasm async events short-lived

gopherbot pushed a commit that referenced this issue Oct 11, 2019
An extra goroutine is necessary to handle asynchronous events on wasm.
However, we do not want this goroutine to exist all the time.
This change makes it short-lived, so it ends after the asynchronous
event was handled.

Fixes #34768

Change-Id: I24626ff0af9d803a01ebe33fbb584d04d2059a44
Reviewed-on: https://go-review.googlesource.com/c/go/+/200497
Run-TryBot: Richard Musiol <neelance@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@neelance

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@bradfitz Do you want to keep https://golang.org/cl/200438 ?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@neelance, yeah, it's harmless. And I like that it has a deadline now, rather than looping forever. Let's just keep it. There's nothing really wasm-specific in there.

@neelance

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

All right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.