Conversation
Fixed order of closing components. Removed unnecessary logging. Made Close() methods safer. Fixed context in pull sync component.
sbackend123
force-pushed
the
fix/shutdown-node
branch
from
478bfab to
7bed9a2
Compare
pkg/api/api.go
Outdated
| wsWg sync.WaitGroup // wait for all websockets to close on exit | ||
| quit chan struct{} | ||
| wsWg sync.WaitGroup // wait for all websockets to close on exit | ||
| closeOnce sync.Once |
There was a problem hiding this comment.
why is this needed? how come that a quit signal can happen twice?
There was a problem hiding this comment.
I believe that this is the protection of the double closed channel, which is panicing.
There was a problem hiding this comment.
yeah that is clear. the question is why would the channel be closed twice in the first place? it suggests a problem in the calling code no?
There was a problem hiding this comment.
I suppose that it is just a common protection, as there is no way to restrict how many times an exported method is called. I did not notice that the Close method is called twice anywhere, but I suppose that a layer of safety is a good thing here.
| quit chan struct{} | ||
| newPeer chan struct{} | ||
| quit chan struct{} | ||
| closeOnce sync.Once |
janos
left a comment
janos
left a comment
There was a problem hiding this comment.
Good hardening of the close functions. I did not like to write so many similar code for service shutdown or closing and wanted to have even the blocking mechanism, to wait until all the goroutines are closed before the Close() function returns (for consistency and reliability), and I have wrote something like this https://pkg.go.dev/resenje.org/x/shutdown#Graceful. If you think that something could be stolen from there, or to make something better and unified for bee, feel free to investigate, or use.
pkg/api/api.go
Outdated
| wsWg sync.WaitGroup // wait for all websockets to close on exit | ||
| quit chan struct{} | ||
| wsWg sync.WaitGroup // wait for all websockets to close on exit | ||
| closeOnce sync.Once |
There was a problem hiding this comment.
I believe that this is the protection of the double closed channel, which is panicing.
After discussion with @acud decided to remove sync.Once() |
Cool, it is good that there has been a discussion. |
| } | ||
|
|
||
| b.logger.Debug("starting shutdown", "component", component) | ||
| defer b.logger.Debug("finished shutdown", "component", component) |
There was a problem hiding this comment.
maybe it is worth to add to it the time-span it took to shut down the component
3 participants
Fixed order of closing components. Removed unnecessary logging. Made Close() methods safer. Fixed context in pull sync component.
Checklist
Description
Context: see the linked issue
What was done:
Fixed the shutdown order of storages. Previously,
StateStorewas closed beforeLocalStore.LocalStoreusesBatchStore, which depends onStateStore. During shutdown or in-flight work,BatchStorecould accessStateStore, causing failedPut/Getoperations, with a risk of incomplete flush and inconsistent local state.Fixed a context handling issue in the pull sync component. Upon receiving a shutdown signal, the component waited 10 seconds to allow its sub-goroutines to terminate gracefully. However, the sub-goroutines did not receive the context cancellation signal.
Added logs to track how long each component takes to shut down.
Removed warning logs in
libp2pfor context cancellation when the node is shutting down, to avoid log noise.Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5397
Screenshots (if appropriate):
AI Disclosure