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

dgroup's shutdown logging is async #15

Open
LukeShu opened this issue Mar 23, 2021 · 2 comments
Open

dgroup's shutdown logging is async #15

LukeShu opened this issue Mar 23, 2021 · 2 comments

Comments

@LukeShu
Copy link
Contributor

LukeShu commented Mar 23, 2021

dgroup's shutdown logging works by watching the Context and logging when it observes that it becomes Done. Because this is async, the ordering of the shutdown log line might actually come after things caused by the shutdown. This is confusing and makes the logs hard to read.

This should be solvable by wrapping the Context in our own Context implementation that logs before propagating the cancelation to child Contexts.

@LukeShu
Copy link
Contributor Author

LukeShu commented Jul 30, 2021

This should be solvable by wrapping the Context in our own Context implementation that logs before propagating the cancelation to child Contexts.

That's no good, because it might introduce a race where

ctx, cancel := context.WithCancel(ctx)
ctx = loggingCtx{ctx}
cancel()
if ctx.Err() == nil {
    panic("this should not happen")
}

To do it right, I'd have to make cancel() block until the log line is printed. And I have no way to adjust cancel().

@LukeShu
Copy link
Contributor Author

LukeShu commented Aug 1, 2021

I suppose the solution could be to have ctx.Err() block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant