fix: shutdown deadlock #2944
fix: shutdown deadlock #2944
Conversation
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @notanatol)
pkg/goroutine/goroutine.go line 1 at r1 (raw file):
package goroutine
Missing header.
pkg/topology/kademlia/kademlia.go
Outdated
| select { | ||
| case <-timer.C: | ||
| case <-time.After(1 * time.Second): | ||
| k.logger.Debugf("kademlia timer not drained after 1 second, goroutine dump...\n%s\n*** end\n", goroutine.Dump()) |
There was a problem hiding this comment.
why not use pprof.Lookup("goroutine").WriteTo(os.Stdout, 1)?
There was a problem hiding this comment.
fixed it to use pprof
pkg/goroutine/goroutine.go
Outdated
| _ = pprof.Lookup("goroutine").WriteTo(out, 2) | ||
| } | ||
|
|
||
| func Stack() string { |
There was a problem hiding this comment.
this is not used anywhere. do we need the new package to wrap a one-liner?
There was a problem hiding this comment.
not needed, just syntactically looked nice.
removed
pkg/node/node.go
Outdated
| @@ -969,6 +980,9 @@ func (b *Bee) Shutdown(ctx context.Context) error { | |||
|
|
|||
| tryClose(b.apiCloser, "api") | |||
|
|
|||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |||
There was a problem hiding this comment.
this was 15s and now is 5s. it needs to be evaluated whether this is the right value, and what are the side effects of a non-clean api shutdown (since this propagates to other components that are later shut-down.
There was a problem hiding this comment.
Reviewable status: 0 of 11 files reviewed, 4 unresolved discussions (waiting on @acud)
pkg/node/node.go line 673 at r3 (raw file):
Previously, acud (acud) wrote…
this is actually an abnormal state but the select would just cause the software to continue booting, and only later shutting down. i think that actually returning an error here and causing the defer statement to initiate the shutdown would be more correct, no?
yes, returning an error now
pkg/node/node.go line 983 at r3 (raw file):
Previously, acud (acud) wrote…
this was 15s and now is 5s. it needs to be evaluated whether this is the right value, and what are the side effects of a non-clean api shutdown (since this propagates to other components that are later shut-down.
reverted
There was a problem hiding this comment.
Reviewed 6 of 10 files at r1, 2 of 4 files at r3, 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status:
|
@notanatol can you push another commit so that the static analyzer can run again? I adjusted the rules. Should pass now |
There was a problem hiding this comment.
done
Reviewable status: 10 of 11 files reviewed, all discussions resolved (waiting on @acud and @mrekucci)
There was a problem hiding this comment.
Reviewed 6 of 10 files at r1, 1 of 4 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status:
Checklist
Description
Closes #2931
Closes #2926
This change is