Fluffy state network now enabled by default and improve status logs #2640
Conversation
… beacon networks. Create status log loop for portal node. Implement stop functions.
bhartnett
changed the title
fluffy/fluffy.nim
Outdated
| setupForeignThreadGc() | ||
|
|
||
| notice "Got interrupt, Fluffy shutting down..." | ||
| node.stop() |
There was a problem hiding this comment.
Currently this doesn't actually wait for every chronos task to complete because we are using cancelSoon() rather than cancelAndWait() in most places.
There was a problem hiding this comment.
Yes, any close call that does not actual wait for closing anything below should be deprecated and the awaited version should be used.
Like this one in discv5: https://github.com/status-im/nim-eth/blob/master/eth/p2p/discoveryv5/protocol.nim#L1120
There was a problem hiding this comment.
So this would be a bit of changes in portal wire and in the different networks. If you want you can split off those changes from this PR change to activate state network.
There was a problem hiding this comment.
Thanks for sharing that example. I'll follow that pattern. Yes, it's turning out to be a bigger change than I expected so I'll do it in two PRs.
| @@ -320,8 +320,7 @@ proc start*(self: var LightClientManager) = | |||
| doAssert self.loopFuture == nil | |||
| self.loopFuture = self.loop() | |||
|
|
|||
| proc stop*(self: var LightClientManager) {.async: (raises: []).} = | |||
There was a problem hiding this comment.
All the stop calls should remain async so that they can be awaited.
Else closing them down without awaiting in the ctrl-c handler will not actually ensure proper clean-up, defeating the point of having a graceful shutdown in the first place.
There was a problem hiding this comment.
Yes, thanks. I'm working on fixing this now. I'm planning to make all the cancel calls use closeAndWait.
There was a problem hiding this comment.
Will revert this for now
fluffy/fluffy.nim
Outdated
| node.stop() | ||
| quit QuitSuccess | ||
|
|
||
| setControlCHook(controlCHandler) |
There was a problem hiding this comment.
I think a raw exception that might be thrown here needs to be catched?
At least that is what is being done in nimbus-eth2: https://github.com/status-im/nimbus-eth2/blob/bf4abf8b9e07c35442b00966f51cc9af5857af33/beacon_chain/nimbus_beacon_node.nim#L2264
There was a problem hiding this comment.
Thanks, I'll look into that
bhartnett
changed the title
|
I've reverted the graceful shutdown changes which will now be part of my next PR. |
| info "History network status", | ||
| radiusPercentage = radiusPercentage.toString(10) & "%", | ||
| radius = n.portalProtocol.dataRadius().toHex(), | ||
| dbSize = $(n.contentDB.size() div 1000) & "kb", |
There was a problem hiding this comment.
@kdeme Did you have any concerns about moving this log out into Portal node? I thought it would make sense to do so because these fields are based on the contentDb and are shared between all the portal sub networks.
Changes in this PR: