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

cmd/compile: deadlock on syntax error #52127

Closed
tdegris opened this issue Apr 3, 2022 · 9 comments
Closed

cmd/compile: deadlock on syntax error #52127

tdegris opened this issue Apr 3, 2022 · 9 comments
Labels
NeedsFix
Milestone

Comments

@tdegris
Copy link

@tdegris tdegris commented Apr 3, 2022

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

$ go version
go version go1.18 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=""
GOARCH="amd64"
GOBIN="/opt/go/bin"
GOCACHE="/Users/some_user/Library/Caches/go-build"
GOENV="/Users/some_user/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/opt/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/opt/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/some_user/Temp/base16-go/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cz/mpwjzvhs2s5dfkk3nzj_m8b40000gn/T/go-build187003372=/tmp/go-build -gno-record-gcc-switches -fno-common"
GOROOT/bin/go version: go version go1.18 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.18
uname -v: Darwin Kernel Version 21.3.0: Wed Jan  5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_ARM64_T6000
ProductName:	macOS
ProductVersion:	12.2.1
BuildVersion:	21D62
lldb --version: lldb-1316.0.9.41
Apple Swift version 5.6 (swiftlang-5.6.0.323.62 clang-1316.0.20.8)

What did you do?

Compiling github.com/tdegris/base16-go crashes the compiler. Instructions to reproduce the bug:

$ git clone https://github.com/tdegris/base16-go.git
$ cd base16-go/themes
$ git checkout a21c6cb
$ go build ...

What did you expect to see?

A compile error or a successful compilation. Sometimes, the compilation does return the following (correct) syntax errors:

$ go build ...
# github.com/tdegris/base16-go/themes
./base16-3024.go:23:3: syntax error: unexpected ) at end of statement
./base16-apathy.go:23:3: syntax error: unexpected ) at end of statement
./base16-apprentice.go:23:3: syntax error: unexpected ) at end of statement
./base16-ashes.go:23:3: syntax error: unexpected ) at end of statement
./base16-atelier-cave-light.go:23:3: syntax error: unexpected ) at end of stateme
./base16-atelier-cave.go:23:3: syntax error: unexpected ) at end of statement
./base16-atelier-dune-light.go:23:3: syntax error: unexpected ) at end of stateme
./base16-atelier-dune.go:23:3: syntax error: unexpected ) at end of statement
./base16-atelier-estuary-light.go:23:3: syntax error: unexpected ) at end of stat
./base16-atelier-estuary.go:23:3: syntax error: unexpected ) at end of statement
./base16-atelier-estuary.go:23:3: too many errors

What did you see instead?

# github.com/tdegris/base16-go/themes
fatal error: all goroutines are asleep - deadlock!

See the full stack trace.

@tdegris tdegris changed the title go build deadlocks instead of reporting syntax error. go build deadlocks instead of reporting syntax errors. Apr 3, 2022
@ianlancetaylor ianlancetaylor changed the title go build deadlocks instead of reporting syntax errors. cmd/compile: deadlock on syntax error Apr 3, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 3, 2022

CC @mdempsky @josharian

@ianlancetaylor ianlancetaylor added the NeedsFix label Apr 3, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone Apr 3, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 4, 2022

Change https://go.dev/cl/398014 mentions this issue: cmd/compile: deadlock on syntax error

@hopehook
Copy link
Contributor

@hopehook hopehook commented Apr 5, 2022

I wrote a demo that reproduces the problem.

package main

import (
	"fmt"
)

const (
	FileNames     = 5
	OpenFileLimit = 1 // if BufferCount == 0 and OpenFileLimit < FileNames, deadlock may occur
	BufferCount   = 0 // if BufferCount > 0, there is no deadlock for range p.err
)

type noder struct {
	err chan string
}

func main() {
	sem := make(chan struct{}, OpenFileLimit)
	noders := make([]*noder, FileNames)

	i := 0
	for i < FileNames {
		p := noder{
			err: make(chan string, BufferCount),
		}
		noders[i] = &p
		go func(c int) {
			sem <- struct{}{}
			defer func() { <-sem }()
			defer close(p.err)
			text := fmt.Sprintf("go func: %d finished", c)
			p.err <- text
			fmt.Println(text)
		}(i)
		i = i + 1
	}

	for _, p := range noders {
		for e := range p.err {
			fmt.Println("received:", e)
		}
	}
}

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Apr 5, 2022

Thanks. I don't think changing the error channel to buffered fixes the issue though. It'll just push the deadlock to only affect when there are files with multiple syntax errors.

I think instead we need to prioritize that the semaphore is use to parse the first N files, rather than just any N.

I think this can be fixed by acquiring the semaphore in the loop outside of the goroutine. But then the loop itself needs to be on a separate goroutine too.

@hopehook
Copy link
Contributor

@hopehook hopehook commented Apr 6, 2022

Thanks. I don't think changing the error channel to buffered fixes the issue though. It'll just push the deadlock to only affect when there are files with multiple syntax errors.

I think instead we need to prioritize that the semaphore is use to parse the first N files, rather than just any N.

I think this can be fixed by acquiring the semaphore in the loop outside of the goroutine. But then the loop itself needs to be on a separate goroutine too.

Thanks.
I'm thinking about your plan carefully.

@hopehook
Copy link
Contributor

@hopehook hopehook commented Apr 6, 2022

I wrote another demo to show my understanding:

package main

import (
	"context"
	"fmt"
	"golang.org/x/sync/semaphore"
)

const (
	FileNames     = 5
	OpenFileLimit = 1
	BufferCount   = 1 // if BufferCount > 0, there is no deadlock for range p.err
)

type noder struct {
	err chan string
}

func main() {
	ctx := context.TODO()
	sem := semaphore.NewWeighted(int64(OpenFileLimit))
	noders := make([]*noder, FileNames)

	for i := range noders {
		if err := sem.Acquire(ctx, 1); err != nil {
			fmt.Printf("Failed to acquire semaphore: %v", err)
			break
		}

		go func(c int) {
			defer sem.Release(1)
			p := noder{
				err: make(chan string, BufferCount),
			}
			noders[c] = &p
			defer close(p.err)
			text := fmt.Sprintf("The goroutine func: %d finished", c)
			p.err <- text
			fmt.Println(text)
		}(i)
	}

	if err := sem.Acquire(ctx, int64(OpenFileLimit)); err != nil {
		fmt.Printf("Failed to acquire semaphore: %v", err)
	}

	for _, p := range noders {
		for e := range p.err {
			fmt.Println("Received:", e)
		}
	}
}

Or

package main

import (
	"fmt"
	"sync"
)

const (
	FileNames     = 5
	OpenFileLimit = 1
	BufferCount   = 1 // if BufferCount > 0, there is no deadlock for range p.err
)

type noder struct {
	err chan string
}

func main() {
	var wg sync.WaitGroup
	sem := make(chan struct{}, OpenFileLimit)
	noders := make([]*noder, FileNames)

	for i := range noders {
		wg.Add(1)
		go func(c int) {
			sem <- struct{}{}
			defer func() {
				<-sem
				wg.Done()
			}()
			p := noder{
				err: make(chan string, BufferCount),
			}
			noders[c] = &p
			defer close(p.err)
			text := fmt.Sprintf("The goroutine func: %d finished", c)
			p.err <- text
			fmt.Println(text)
		}(i)
	}

	wg.Wait()

	for _, p := range noders {
		for e := range p.err {
			fmt.Println("Received:", e)
		}
	}
}

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Apr 6, 2022

Here's the code I had in mind: https://go.dev/play/p/yKyrv7NiVB9

@hopehook
Copy link
Contributor

@hopehook hopehook commented Apr 6, 2022

Here's the code I had in mind: https://go.dev/play/p/yKyrv7NiVB9

Thanks, I hadn't thought of that.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 11, 2022

Change https://go.dev/cl/399054 mentions this issue: cmd/compile: add a test case and some comments for deadlock on syntax error

gopherbot pushed a commit that referenced this issue Apr 12, 2022
… error

After CL 398014 fixed a compiler deadlock on syntax errors,
this CL adds a test case and more details for that.

How it was fixed:

CL 57751 introduced a channel "sem" to limit the number of
simultaneously open files.

Unfortunately, when the number of syntax processing goroutines
exceeds this limit, will easily trigger deadlock.

In the original implementation, "sem" only limited the number
of open files, not the number of concurrent goroutines, which
will cause extra goroutines to block on "sem". When the p.err
of the following iteration happens to be held by the blocking
goroutine, it will fall into a circular wait, which is a deadlock.

CL 398014 fixed the above deadlock, also see issue #52127.

First, move "sem <- struct{}{}" to the outside of the syntax
processing goroutine, so that the number of concurrent goroutines
does not exceed the number of open files, to ensure that all
goroutines in execution can eventually write to p.err.

Second, move the entire syntax processing logic into a separate
goroutine to avoid blocking on the producer side.

Change-Id: I1bb89bfee3d2703784f0c0d4ded82baab2ae867a
Reviewed-on: https://go-review.googlesource.com/c/go/+/399054
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

5 participants