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

x/playground: show a meaningful error message for build timeouts #38052

Closed
leighmcculloch opened this issue Mar 24, 2020 · 19 comments
Closed

x/playground: show a meaningful error message for build timeouts #38052

leighmcculloch opened this issue Mar 24, 2020 · 19 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@leighmcculloch
Copy link
Contributor

leighmcculloch commented Mar 24, 2020

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.

package main

import (
	"fmt"

	"github.com/stellar/go/txnbuild"
)

func main() {
	tx := txnbuild.Transaction{}
	err := tx.Build()
	if err != nil {
		fmt.Println(err)
		return
	}
	fmt.Println(tx)
}
--go.mod--
module play.ground
require github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3

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.

go: finding github.com/stellar/go latest
go: downloading github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: extracting github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: downloading github.com/pkg/errors v0.8.1
go: downloading golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708
go: downloading github.com/lib/pq v1.2.0
go: downloading github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
go: extracting github.com/pkg/errors v0.8.1
go: extracting github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
go: extracting github.com/lib/pq v1.2.0
go: extracting golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708
go: finding github.com/pkg/errors v0.8.1
go: finding github.com/lib/pq v1.2.0
go: finding github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
go: finding golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708

Go build failed.
@gopherbot gopherbot added this to the Unreleased milestone Mar 24, 2020
@FiloSottile
Copy link
Contributor

It doesn't look like finding x/crypto is the problem here. There is some other build error after that that isn't logged. See this example with a different output: https://play.golang.org/p/D1_5KTesTr0

@leighmcculloch
Copy link
Contributor Author

@FiloSottile What output do you see? I see the same output when I run your link.

go: finding github.com/stellar/go latest
go: downloading github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: extracting github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: downloading golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708
go: downloading github.com/pkg/errors v0.8.1
go: downloading github.com/lib/pq v1.2.0
go: downloading github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
go: extracting github.com/pkg/errors v0.8.1
go: extracting github.com/lib/pq v1.2.0
go: extracting github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
go: extracting golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708
go: finding github.com/pkg/errors v0.8.1
go: finding github.com/lib/pq v1.2.0
go: finding github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
go: finding golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708

Go build failed.

@leighmcculloch
Copy link
Contributor Author

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.

go: finding golang.org/x/crypto latest
go: downloading golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59
go: extracting golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59
build command-line-arguments: cannot load golang.org/x/crypto: module golang.org/x/crypto@latest found (v0.0.0-20200323165209-0ec3e9974c59), but does not contain package golang.org/x/crypto

Go build failed.

@leighmcculloch
Copy link
Contributor Author

leighmcculloch commented Mar 24, 2020

🤦‍♂ That was silly of me. There are no .go files in the root of the crypto project, so that ☝️ error is irrelevant.

@FiloSottile
Copy link
Contributor

I see this at https://play.golang.org/p/D1_5KTesTr0

go: finding github.com/stellar/go latest
go: downloading github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: extracting github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3

@leighmcculloch
Copy link
Contributor Author

leighmcculloch commented Mar 24, 2020

Maybe this is purely a timeout issue?

@bcmills
Copy link
Contributor

bcmills commented Mar 25, 2020

The --go.mod-- line is invalid: it is missing spaces around the -- tokens.

But we should be producing a better error message for that.

@bcmills
Copy link
Contributor

bcmills commented Mar 25, 2020

FWIW, here's what I get on the command line:

play.ground$ txtar -x > main.go <<EOF
package main

import (
        "fmt"

        "github.com/stellar/go/txnbuild"
)

func main() {
        tx := txnbuild.Transaction{}
        err := tx.Build()
        if err != nil {
                fmt.Println(err)
                return
        }
        fmt.Println(tx)
}
--go.mod--
module play.ground
require github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
EOF

play.ground$ go build .
go: finding module for package github.com/stellar/go/txnbuild
go: downloading github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: found github.com/stellar/go/txnbuild in github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: downloading github.com/pkg/errors v0.8.1
go: downloading github.com/lib/pq v1.2.0
go: downloading golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708
go: downloading github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
# play.ground
./main.go:18:1: syntax error: non-declaration statement outside function body

play.ground$ cat main.go
package main

import (
        "fmt"

        "github.com/stellar/go/txnbuild"
)

func main() {
        tx := txnbuild.Transaction{}
        err := tx.Build()
        if err != nil {
                fmt.Println(err)
                return
        }
        fmt.Println(tx)
}
--go.mod--
module play.ground
require github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3

play.ground$

@bcmills bcmills changed the title x/playground: Go build failed finding golang.org/x/crypto x/playground: missing stderr output for https://play.golang.org/p/SK8gXGPzUvf Mar 25, 2020
@bcmills
Copy link
Contributor

bcmills commented Mar 25, 2020

Hmm, but fixing that doesn't fix the Playground build. There is something else going on too.

@bcmills
Copy link
Contributor

bcmills commented Mar 25, 2020

https://play.golang.org/p/-YW3xkuXIm1 fails with the same inscrutable Go build failed. on the playground, but builds fine locally using go 1.14.1:

play.ground$ txtar -x >main.go <<EOF
> package main

import (
        "fmt"

        "github.com/stellar/go/txnbuild"
)

func main() {
        tx := txnbuild.Transaction{}
        err := tx.Build()
        if err != nil {
                fmt.Println(err)
                return
        }
        fmt.Println(tx)
}
-- go.mod --
module play.ground

go 1.13

require github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
EOF

play.ground$ go build .
go: downloading github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: downloading github.com/pkg/errors v0.8.1
go: downloading golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708
go: downloading github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
go: downloading github.com/lib/pq v1.2.0

play.ground$

@bcmills
Copy link
Contributor

bcmills commented Mar 25, 2020

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.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 25, 2020
@andybons
Copy link
Member

@toothrot has offered to take a look.

@toothrot
Copy link
Contributor

The good news is this is reproducing when running the playground locally. I'm still looking into it.

@toothrot toothrot self-assigned this Mar 26, 2020
@toothrot
Copy link
Contributor

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

@gopherbot
Copy link

Change https://golang.org/cl/227352 mentions this issue: playground: move build and run into functions

@gopherbot
Copy link

Change https://golang.org/cl/227350 mentions this issue: playground: use the correct contexts for sandbox requests

gopherbot pushed a commit to golang/playground that referenced this issue Apr 8, 2020
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 <andybons@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit to golang/playground that referenced this issue Apr 15, 2020
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 <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
@toothrot toothrot changed the title x/playground: missing stderr output for https://play.golang.org/p/SK8gXGPzUvf x/playground: show a meaningful error message for build timeouts Apr 15, 2020
@gopherbot
Copy link

Change https://golang.org/cl/228438 mentions this issue: playground: show a meaningful error on build timeout

@toothrot toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 16, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 16, 2020
gopherbot pushed a commit to golang/playground that referenced this issue Apr 21, 2020
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 <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@toothrot
Copy link
Contributor

This snippet (https://play.golang.org/p/jRB81AYhnq8) now fails correctly:

timeout running go build
go: downloading github.com/stellar/go v0.0.0-20200320201948-bab6abc8fbf3
go: downloading github.com/pkg/errors v0.8.1
go: downloading github.com/stellar/go-xdr v0.0.0-20180917104419-0bc96f33a18e
go: downloading github.com/lib/pq v1.2.0
go: downloading golang.org/x/crypto v0.0.0-20191112222119-e1110fd1c708

Go build failed.

@toothrot
Copy link
Contributor

Thanks again for the helpful issue report!

@golang golang locked and limited conversation to collaborators Apr 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants