Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core/tracing: Add OnClose Trace Hook #29629

Merged
merged 2 commits into from
Apr 24, 2024
+8 −0
Merged

core/tracing: Add OnClose Trace Hook #29629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e84d87
add OnClose Hook which triggered on blockchain Stop
ziogaschr Apr 23, 2024
e881bf7
call logger.OnClose before triedb.Close
ziogaschr Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions core/blockchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,6 +1153,10 @@ func (bc *BlockChain) Stop() {
}
}
}
// Allow tracers to clean-up and release resources.
if bc.logger != nil && bc.logger.OnClose != nil {
bc.logger.OnClose()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by e881bf7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it depends what you mean by tracing open. Instance is created before triedb. But "OnBlockchainInit" is called after triedb is created.

// Close the trie database, release all the held resources as the last step.
if err := bc.triedb.Close(); err != nil {
log.Error("Failed to close trie database", "err", err)
Expand Down
4 changes: 4 additions & 0 deletions core/tracing/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow returning an error? In general Go's Close() methods do so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the other tracing methods return an error or any value for that matter. My 2c keep it as is.


// BlockStartHook is called before executing `block`.
// `td` is the total difficulty prior to `block`.
BlockStartHook = func(event BlockEvent)
Expand Down Expand Up @@ -153,6 +156,7 @@ type Hooks struct {
OnGasChange GasChangeHook
// Chain events
OnBlockchainInit BlockchainInitHook
OnClose CloseHook
OnBlockStart BlockStartHook
OnBlockEnd BlockEndHook
OnSkippedBlock SkippedBlockHook
Expand Down