-
Notifications
You must be signed in to change notification settings - Fork 18k
x/playground: show a meaningful error message for build timeouts #38052
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
Comments
It doesn't look like finding |
@FiloSottile What output do you see? I see the same output when I run your link.
|
Ahh, I think we might be on to something. Check this out if I import the crypto package directly: https://play.golang.org/p/pcRtlFF7Ihs.
|
🤦♂ That was silly of me. There are no .go files in the root of the crypto project, so that ☝️ error is irrelevant. |
I see this at https://play.golang.org/p/D1_5KTesTr0
|
Maybe this is purely a timeout issue? |
The But we should be producing a better error message for that. |
FWIW, here's what I get on the command line:
|
Hmm, but fixing that doesn't fix the Playground build. There is something else going on too. |
https://play.golang.org/p/-YW3xkuXIm1 fails with the same inscrutable
|
CC @andybons @dmitshur @toothrot @cagedmantis: do y'all maintain the Playground these days? (The entry for it at https://dev.golang.org/owners is blank; I know it used to be mainly @bradfitz.) CC @ysmolsky who I think has done some work on it too. |
@toothrot has offered to take a look. |
The good news is this is reproducing when running the playground locally. I'm still looking into it. |
Ok, after lots of cleanup and investigation, there's a bug in the playground. The build for this code is timing out, but the wrong context is being checked for the timeout error. I'll have a patch ready for this soon, along with removing NaCL code as part of #25224 |
Change https://golang.org/cl/227352 mentions this issue: |
Change https://golang.org/cl/227350 mentions this issue: |
The sandbox code was incorrectly using the request context instead of the build context when trying to determine if there was a DeadlineExceeded error. This would manifest to the user as a blank response in the build output, rather than the correct error. Additionally, the sandbox code was using the incorrect context for running the binary. This means we were not correctly enforcing maxRunTime. Finally, tests do not pass with a maxRunTime of 2 seconds on my machine, and it's unclear what impact enforcing this would have on production code. I've increased it to 5 seconds. It would be prudent to add metrics here to determine how user programs are impacted in a follow-up issue. Updates golang/go#25224 Updates golang/go#38052 Change-Id: I59aa8caeb63a9eec687bfbe4f69c57f71a13440d Reviewed-on: https://go-review.googlesource.com/c/playground/+/227350 Reviewed-by: Andrew Bonventre <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]>
Based on a suggestion in CL 226697, this change moves the build and run steps from compileAndRun into their own functions. This will make it less likely to accidentally use the incorrect context again, which was the cause of golang/go#38052. Updates golang/go#25224 Change-Id: Id8399c2bd93fca7d61dced0431c8596f7998f3ee Reviewed-on: https://go-review.googlesource.com/c/playground/+/227352 Run-TryBot: Alexander Rakoczy <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]> Reviewed-by: Carlos Amedee <[email protected]>
Change https://golang.org/cl/228438 mentions this issue: |
Add support for returning a meaningful error message to the user, along with partial build output if present. This change also improves graceful command termination throughout the playground. It introduces internal.WaitOrStop, which will wait for a command to finish, or gracefully stop the command if it has not finished by the time the context is cancelled. This also resolves comments from CL 227351. Updates golang/go#38052 Updates golang/go#38530 Updates golang/go#25224 Change-Id: I3a36ad2c5f3626c9cd5b3c1139543ccde3e17ba1 Reviewed-on: https://go-review.googlesource.com/c/playground/+/228438 Run-TryBot: Alexander Rakoczy <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
This snippet (https://play.golang.org/p/jRB81AYhnq8) now fails correctly:
|
Thanks again for the helpful issue report! |
What version of Go are you using (
go version
)?The version of Go in use by the play.golang.org.
Does this issue reproduce with the latest release?
N/A
What operating system and processor architecture are you using (
go env
)?Go Playground.
What did you do?
Import a module that imports
golang.org/x/crypto
.https://play.golang.org/p/SK8gXGPzUvf
What did you expect to see?
I expected to see the playground's Go build process find
golang.org/x/crypto
and continue building, or if there is a build failure, a more meaningful error message.What did you see instead?
When finding
golang.org/x/crypto
the build fails. No meaningful error message beyond that is displayed. Timeout is not mentioned so I don't think the process is taking too long and being killed.The text was updated successfully, but these errors were encountered: