core/tracing: Add OnClose Trace Hook #29629
Conversation
ziogaschr
requested review from
karalabe,
holiman and
rjl493456442
as code owners
| @@ -107,6 +107,9 @@ type ( | |||
| // BlockchainInitHook is called when the blockchain is initialized. | |||
| BlockchainInitHook = func(chainConfig *params.ChainConfig) | |||
|
|
|||
| // CloseHook is called when the blockchain closes. | |||
| CloseHook = func() | |||
There was a problem hiding this comment.
Do we want to allow returning an error? In general Go's Close() methods do so
There was a problem hiding this comment.
Good point.
The initial idea was to be used for allowing tracers to cleanup their stuff. I also checked the errors in Blockchain.Stop() and I am not sure if tracer will make any use of them.
There was a problem hiding this comment.
None of the other tracing methods return an error or any value for that matter. My 2c keep it as is.
| // Allow tracers to clean-up and release resources. | ||
| if bc.logger != nil && bc.logger.OnClose != nil { | ||
| bc.logger.OnClose() | ||
| } |
There was a problem hiding this comment.
Out of curiosity, do we want to close the logger first or the triedb first? Any change the logger might have access to the trie and might want to do something with it? (Or any other live object really).
There was a problem hiding this comment.
Closing should be reverse order to start. I assume triedb was opened before the tracer, so we should Close the tracer before we close the triedb, IMO.
There was a problem hiding this comment.
The tracer initiates before the triedb. On the other hand, and just in case a future tracer wants to finish reading something from triedb, it might feel safer to call OnClose before.
There was a problem hiding this comment.
Any change the logger might have access to the trie and might want to do something with it?
Tracer has indirect access via statedb. However the lifetime of a statedb instance is that of a block.
On the other hand, and just in case a future tracer wants to finish reading something from triedb
People really shouldn't keep a reference to statedb which they could use at OnClose. Because a new instance is created at every block.
@ziogaschr I think the reverse order martin mentioned makes sense. Can you please change it?
There was a problem hiding this comment.
I assume triedb was opened before the tracer, so we should Close the tracer before [...] triedb
The tracer initiates before the triedb.
I think the reverse order martin mentioned makes sense. Can you please change it?
This doesn't compute for me... If the tracer opens before the triedb, then it should close after triedb.
Ideal order:
a.open
b.open
b.close
a.close
There was a problem hiding this comment.
I mean, I don't particularly care if we do this way or another really, just pointing out that there seems to have been some misunderstanding
There was a problem hiding this comment.
Well it depends what you mean by tracing open. Instance is created before triedb. But "OnBlockchainInit" is called after triedb is created.
The OnClose trace hook is being triggered on blockchain Stop, so as tracers can release any resources.
The OnClose trace hook is being triggered on blockchain Stop, so as tracers can release any resources.
The OnClose trace hook is being triggered on blockchain Stop, so as tracers can release any resources.
The
OnClosetrace hook is being triggered on blockchain Stop, so as tracers can release any resources.