Allways close chainsync servers reader state #2880
Conversation
There was a problem hiding this comment.
Excellent.
It's probably a general thing we should clarify in our docs for typed protocols and the network framework etc, and also audit in our existing code:
Do not allocate resources within a protocol handler. Have all resources allocated (with appropriate bracketing) and passed into the protocol handler. Protocol handlers get killed unceremoniously, and the (continuation) style in which they are written does not support resource cleanup.
| @@ -41,13 +50,12 @@ chainSyncHeadersServer | |||
| ) | |||
| => Tracer m (TraceChainSyncServerEvent blk) | |||
| -> ChainDB m blk | |||
| -> Reader m blk (WithPoint blk (SerialisedHeader blk)) | |||
There was a problem hiding this comment.
Yes great, pass the resource in, rather than allocating it within the protocol handler.
| bracket | ||
| (chainSyncHeaderServerReader (getChainDB kernel) registry) | ||
| ChainDB.readerClose |
karknu
force-pushed
the
karknu/chainsync_leak
branch
from
a6a6ca7
to
3d66c2e
Compare
| -> m (Reader m blk (WithPoint blk (Serialised blk))) | ||
| chainSyncBlockServerReader chainDB registry = ChainDB.newReader chainDB registry getSerialisedBlockWithPoint | ||
|
|
||
|
|
There was a problem hiding this comment.
If you have another reason to update this PR, please also remove these extraneous newlines while you're at it.
|
@karknu Is this PR ready for merge? I see that Duncan Approved. You said it slowed the leak, so I'm happy to get it in before the upcoming release. I hope to open a PR that supersedes today to also beat the release, but I might lose that race. So let's have your improvement in place ASAP. Thanks. |
|
(The failing windows CI build does not currently block merge, especially in light of this unrelated diff.) |
karknu
force-pushed
the
karknu/chainsync_leak
branch
from
3d66c2e
to
1932e92
Compare
Rebased and will merge. |
karknu
force-pushed
the
karknu/chainsync_leak
branch
from
1932e92
to
39f8d9c
Compare
|
bors merge |
|
Build succeeded: |
No description provided.