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.

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

eth/filters, eth/tracers: add request cancellation checks #26320

Merged
merged 5 commits into from
Dec 15, 2022

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Dec 6, 2022

This ensures that RPC method handlers will react to a timeout or cancelled request soon after the event occurs.

This ensures that RPC method handlers will react to a timeout or
cancelled request soon after the event occurs.
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

case jobs <- task:
case <-ctx.Done():
failed = ctx.Err()
break
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Classic mistake here, break here will terminate the select instead of enclosing loop.

for i, tx := range txs {
// Send the trace task over for execution
jobs <- &txTraceTask{statedb: statedb.Copy(), index: i}
task := &txTraceTask{statedb: statedb.Copy(), index: i}
select {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this more efficient than simply checking ctx.Err() at the beginning of the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not more efficient, I just did it because the send could theoretically block at this point. Canceling the context would unblock it. However, it seems the jobs channel is actually buffered, and so the send will always succeed.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.11.0 milestone Dec 15, 2022
@holiman holiman merged commit f53ff0f into ethereum:master Dec 15, 2022
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
…6320)

This ensures that RPC method handlers will react to a timeout or
cancelled request soon after the event occurs.


Co-authored-by: Sina Mahmoodi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants