Properly shut down a node, its DBs, and its threads #1488
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrBliss
added
consensus
mrBliss
force-pushed
the
mrBliss/fix-1470
branch
2 times, most recently
from
8b7c69d
to
0f303ad
Compare
mrBliss
commented
Jan 23, 2020
| :: IOLike m | ||
| => CacheEnv m hash h | ||
| -> EpochNo -- ^ The new current epoch | ||
| -> m () | ||
| reset cacheEnv@CacheEnv { hasFS, err, hashInfo, registry, cacheVar } epoch = do | ||
| restart cacheEnv epoch = do |
There was a problem hiding this comment.
Add a comment that this is used only in the tests (it doesn't leak past the ImmutableDB, because the only code path that triggers it is part of the Internalr ecord) and that we don't have to worry about race conditions involving bgThreadVar.
| -- and the registry, so we must use 'unsafeReleaseAll' instead of | ||
| -- 'releaseAll'. | ||
| (DbClosed, unsafeReleaseAll _dbRegistry) | ||
| (DbClosed, cleanUp hasFS st) |
There was a problem hiding this comment.
Let open return an OpenState and close take one, so that we can make sure that we only clean up an OpenState. I.e., we cannot have accidentally closed the state already before trying to clean it up.
Same for withOpenState.
edsko
approved these changes
Jan 23, 2020
Fixes #1470. * Don't use `releaseAll` or `unsafeReleaseAll` in the ImmutableDB, as that will terminate all background threads, not just the resources using the ImmutableDB. Instead, use `Index.close` to shut down the background thread. * Start and close the ChainDB using the `ResourceRegistry` instead of using `withChainDB`, this makes sure that resources are released in the right order. Previously, the ChainDB was being closed before the background threads using it were terminated, resulting in `ClosedDBError`s.
Previously, `open` and `close` returned/took an `InternalState` which could either be a `ClosedState` or an `OpenState`. Now, we return/take an `InternalState` so that in `close`, we can't accidentally pass an already closed state to `cleanUp` instead of the previous open state.
mrBliss
force-pushed
the
mrBliss/fix-1470
branch
from
5d45380
to
bf4d1c4
Compare
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Jan 23, 2020
1488: Properly shut down a node, its DBs, and its threads r=mrBliss a=mrBliss Fixes #1470. * Don't use `releaseAll` or `unsafeReleaseAll` in the ImmutableDB, as that will terminate all background threads, not just the resources using the ImmutableDB. Instead, use `Index.close` to shut down the background thread. * Start and close the ChainDB using the `ResourceRegistry` instead of using `withChainDB`, this makes sure that resources are released in the right order. Previously, the ChainDB was being closed before the background threads using it were terminated, resulting in `ClosedDBError`s. Co-authored-by: Thomas Winant <thomas@well-typed.com>
|
bors r- |
Canceled |
|
🙄 |
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Jan 23, 2020
1488: Properly shut down a node, its DBs, and its threads r=mrBliss a=mrBliss Fixes #1470. * Don't use `releaseAll` or `unsafeReleaseAll` in the ImmutableDB, as that will terminate all background threads, not just the resources using the ImmutableDB. Instead, use `Index.close` to shut down the background thread. * Start and close the ChainDB using the `ResourceRegistry` instead of using `withChainDB`, this makes sure that resources are released in the right order. Previously, the ChainDB was being closed before the background threads using it were terminated, resulting in `ClosedDBError`s. 1496: Address review feedback on #1425. r=edsko a=edsko Co-authored-by: Thomas Winant <thomas@well-typed.com> Co-authored-by: Edsko de Vries <edsko@well-typed.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1470.
Don't use
releaseAllorunsafeReleaseAllin the ImmutableDB, as that will terminate all background threads, not just the resources using the ImmutableDB. Instead, useIndex.closeto shut down the background thread.Start and close the ChainDB using the
ResourceRegistryinstead of usingwithChainDB, this makes sure that resources are released in the right order. Previously, the ChainDB was being closed before the background threads using it were terminated, resulting inClosedDBErrors.