Skip to content

slog: Wrapping default handler causes deadlock #62424

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

Closed
mkch opened this issue Sep 2, 2023 · 6 comments
Closed

slog: Wrapping default handler causes deadlock #62424

mkch opened this issue Sep 2, 2023 · 6 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mkch
Copy link

mkch commented Sep 2, 2023

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

$ go version
go version go1.21.0 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=''
GOCACHE='/Users/me/Library/Caches/go-build'
GOENV='/Users/me/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/me/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/me/go'
GOPRIVATE=''
GOPROXY='https://goproxy.cn,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/4v/g652j2zn37lg22crrdwmcqdm0000gn/T/go-build1896165565=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

https://go.dev/play/p/gu5St1Epba_9

What did you expect to see?

Log message like the following:
2023/09/02 10:24:00 INFO debug message

What did you see instead?

fatal error: all goroutines are asleep - deadlock!

@panjf2000
Copy link
Member

Check out the comment in slog.SetDefault

func SetDefault(l *Logger) {
defaultLogger.Store(l)
// If the default's handler is a defaultHandler, then don't use a handleWriter,
// or we'll deadlock as they both try to acquire the log default mutex.
// The defaultHandler will use whatever the log default writer is currently
// set to, which is correct.
// This can occur with SetDefault(Default()).
// See TestSetDefault.

@mkch
Copy link
Author

mkch commented Sep 2, 2023

Check out the comment in slog.SetDefault

func SetDefault(l *Logger) {
defaultLogger.Store(l)
// If the default's handler is a defaultHandler, then don't use a handleWriter,
// or we'll deadlock as they both try to acquire the log default mutex.
// The defaultHandler will use whatever the log default writer is currently
// set to, which is correct.
// This can occur with SetDefault(Default()).
// See TestSetDefault.

Sorry, I can't see any relevance.

@panjf2000
Copy link
Member

panjf2000 commented Sep 2, 2023

slog.SetDefault can only prevent the loggers that use the exact defaultHandler from deadlock, and it doesn't handle the case you provided (embed the default handler into a struct), I'm not sure whether we should fix this as it seems like a rare case.

/cc @jba

@panjf2000 panjf2000 added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 2, 2023
@panjf2000 panjf2000 added this to the Backlog milestone Sep 2, 2023
@mkch
Copy link
Author

mkch commented Sep 2, 2023

slog.SetDefault can only prevent the deadlock from the loggers that use the exact defaultHandler, and it doesn't handle the case you provided (embed the default handler into a struct), I'm not sure whether we should fix this as it seems like a rare case.

/cc @jba

Thanks for your clarification. Can I consider this a implementation limitation?
What I really want to do is changing the level of the default Handler. Something lile https://go.dev/play/p/bXoD9C49mNy

@panjf2000
Copy link
Member

Looks like #62418 will work for you once it's accepted and implemented.

@seankhliao
Copy link
Member

Duplicate of #61892

@seankhliao seankhliao marked this as a duplicate of #61892 Sep 2, 2023
@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2023
@golang golang locked and limited conversation to collaborators Sep 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants