log/slog: LevelHandler example is invalid; results in a deadlock #61892
Comments
|
CC @jba |
mknyszek
added
the
NeedsInvestigation
|
The deadlock is masking a deeper problem. There is an inherent circularity here. In addition to setting slog's default handler,
We need to find a way to resolve that circularity while preserving existing behavior. I'll be out for two weeks, but I'll revisit this when I return. |
|
I lied, I'm still here. Just to be clear, this is not about how the code is currently written. If you're thinking about how we can reorganize the code in Until we think of a better answer (if there is one), here is a simple workaround: break the connection between log and slog. After calling |
|
@jba, I very much appreciate the workaround! That works for our projects. Should I leave the issue open until the example is fixed or is that going to be done either way when updating |
|
Let's leave it open until we have a satisfying answer, or decide that that is impossible. |
|
I came to the same issue while just wanting to increase the level of the default slog handler. It seems a pretty expected use case. Why not just simply add a third exported In my case just discovering the package, I was expecting I also find weird to me that wrapping the |
Actually it does not do the same, as it strips the leading timestamp: https://go.dev/play/p/F-4rvQ99QgV |
|
@atuleu, yes, the flags has to be set, too: log.SetFlags(log.Ldate|log.Ltime) |
seankhliao
changed the title
|
I found a (dirty) workaround for wrapping the default handler, which was probably unintended. More details in here: #56345 (comment) type WrapperHandler struct {
slog.Handler
}
func (w *WrapperHandler) Handle(ctx context.Context, record slog.Record) error {
// doSomething()
return w.Handler.Handle(ctx, record)
}
func main() {
wrap := &WrapperHandler(slog.Default().Handler())
*slog.Default() = *slog.New(wrap) // Bypass SetDefault using pointer hacks (not thread-safe obviously)
slog.Info("TEST") // Works just fine
} |
If that's something too deep to change, there should be a way of setting slog's default logger without touching log's default logger, or directly changing the default handler. The comment above shows that would be enough. Having to call But also, why is slog using log.Logger on default settings? Wouldn't a definitive solution be for slog to be its own independent thing, and maybe add some function that users can explicitly call to bridge the gap between log and slog? It could be something as simple as Another possibility would be adding a Admittedly I still haven't looked too deep into the source code, but I can imagine a dozen different ways that this can be resolved, all with their pros and cons, but it doesn't look impossible. |
|
I can confirm the above works, thanks. needless to say its an awful hack, but I think thats more the fault of the current awkward API. here is another example: package main
import (
"context"
"log/slog"
)
type debug struct {
slog.Handler
}
func (debug) Enabled(_ context.Context, level slog.Level) bool {
return level >= slog.LevelDebug
}
func main() {
h := slog.Default().Handler()
*slog.Default() = *slog.New(debug{h})
slog.Debug("hello world")
} |
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
https://go.dev/play/p/_usbsfWCk4k
What did you expect to see?
The example from here to compile with the final
log/slog.That example to work with the final
log/slogwithout known deadlocks.What did you see instead?
The example had to be modified to include
context.Contexts, see the code in the playground.The code still results in:
In the original issue, @jba had said that it should be possible to fix this deadlock once
slogis imported into stdlib, but it seems like a test case for this example hadn't been made?Additional information
Originally, my need for this code is from the desire to use the default plain-text handler of
log/slogwith debug level.The text was updated successfully, but these errors were encountered: