el_manager initial refactor. #6228
Conversation
cheatfate
force-pushed
the
el-refactoring
branch
from
6e019b9
to
763e631
Compare
| asyncSpawn connection.close() | ||
| connection.web3 = none[Web3]() | ||
| of Degraded: | ||
| await connection.close() |
There was a problem hiding this comment.
Could this increase the delay until a fallback client is used? close may run for 30s, and with await no progress is done during that time.
The new version is at least cleaner in tracking what is going on, the old version possibly run into situation where multiple close processes were running at same time I guess.
There was a problem hiding this comment.
Every possible async task could run for 30s, that's not how we should protect the code from running for 30s.
There was a problem hiding this comment.
Procedures should not spawn procedure with close and do not wait for it actually being closed. As we seen this many times before its not a good practice which leads to, leaks and UB (when just closed transport being reused by OS and you will have 2 transports with same FD in process, one is closing and another was just opened).
cheatfate
force-pushed
the
el-refactoring
branch
2 times, most recently
from
80229c5
to
3baa56b
Compare
|
|
||
| # TODO can't be defined within exchangeConfigWithSingleEL | ||
| func `==`(x, y: Quantity): bool {.borrow.} | ||
|
|
||
| proc exchangeConfigWithSingleEL(m: ELManager, connection: ELConnection) {.async.} = | ||
| proc exchangeConfigWithSingleEL( |
There was a problem hiding this comment.
What is your name proposal for it? It still performs network_id check.
There was a problem hiding this comment.
sure, so checkNetworkIdWithSingleEL, say, or checkChainIdWithSingleEL
| @@ -1763,18 +1949,27 @@ func hasProperlyConfiguredConnection*(m: ELManager): bool = | |||
|
|
|||
| false | |||
|
|
|||
| proc startExchangeTransitionConfigurationLoop(m: ELManager) {.async.} = | |||
| proc startExchangeTransitionConfigurationLoop( | |||
| pending.add(m.chainSyncingLoopFut.cancelAndWait()) | ||
| if not(m.exchangeTransitionConfigurationLoopFut.isNil()) and | ||
| not(m.exchangeTransitionConfigurationLoopFut.finished()): | ||
| pending.add(m.exchangeTransitionConfigurationLoopFut.cancelAndWait()) |
There was a problem hiding this comment.
Do the waits here delay clean database closing in case of stuck connections, or at least allow the database to close cleanly?
There was a problem hiding this comment.
This waits are proper cancellation, you should wait until loops will not be cancelled.
There was a problem hiding this comment.
Ideally, yes. My question is whether in the case where it takes arbitrarily long to finish the loops, whether the database cleanup still happens first. If it doesn't, then it places the user in a less than great position. Ultimately the Chronos state is ephemeral and the state which has to remain intact for the next run is in the database.
There was a problem hiding this comment.
If you want to close database first - do it first, if you want to work properly in async world, you should signal your workers and wait them to complete their own cleanup processes. Overall construction of exit procedure in BN is incorrect, because it does not allow tasks to perform any cleanup procedures. You working with database in non-async way, if you think that you first task is close database - do it before signaling async tasks to finish.
There was a problem hiding this comment.
Fair enough, and out of scope of this PR.
Goals