Skip to content

rpc/client: check the context before sending message #28582

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

Closed
wants to merge 1 commit into from

Conversation

minh-bq
Copy link
Contributor

@minh-bq minh-bq commented Nov 22, 2023

We observe failure in TestEthClient/TxInBlockInterrupted which can be reproduced by

go test -test.v -run=^TestEthClient/TxInBlockInterrupted --count=100

=== RUN   TestEthClient/TxInBlockInterrupted
    ethclient_test.go:399: transaction should be nil
--- FAIL: TestEthClient (0.03s)

The testcase expects to get context cancel error when cancelling context passed to the ethclient call before doing the call. However, the rpc client's send does not guarantee that order. It uses select to wait on sending action and context.Done() simultaneously, so it is possible that the branch of sending action is taken and we get the response without error. This commit adds a non-blocking wait on context.Done() to catch the cancelled context before doing the actually send.

We observe failure in TestEthClient/TxInBlockInterrupted which can be reproduced
by

	go test -test.v -run=^TestEthClient/TxInBlockInterrupted --count=100

	=== RUN   TestEthClient/TxInBlockInterrupted
	    ethclient_test.go:399: transaction should be nil
	--- FAIL: TestEthClient (0.03s)

The testcase expects to get context cancel error when cancelling context passed
to the ethclient call before doing the call. However, the rpc client's send does
not guarantee that order. It uses select to wait on sending action and
context.Done() simultaneously, so it is possible that the branch of sending
action is taken and we get the response without error. This commit adds a
non-blocking wait on context.Done() to catch the cancelled context before doing
the actually send.
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.

I don't want to fix this if it's only to fix a failing test. One might argue that context cancellation should be prioiitized, thus the change is More Correct.
On the other hand, one might argue that if both cases are instantly available, then one does not need to respect the Cancel -- because the context is also passed to write, so we can hand over the decision to the next part.

Not sure what to think here. I think I lean towards preferring the existing code (which would mean that the test needs to be fixed in a different manner)

@minh-bq
Copy link
Contributor Author

minh-bq commented Nov 22, 2023

I agree with that. So the possible solution is to remove the context cancel part in that testcase, just keep this part and rename the testcase

// Test tx in block not found.
if _, err := ec.TransactionInBlock(context.Background(), block.Hash(), 20); err != ethereum.NotFound {
t.Fatal("error should be ethereum.NotFound")
}

@fjl
Copy link
Contributor

fjl commented Nov 22, 2023

Yes, we should fix the test instead.

@minh-bq
Copy link
Contributor Author

minh-bq commented Nov 22, 2023

I've created the fix for the test in #28583

@minh-bq minh-bq closed this Nov 22, 2023
@minh-bq minh-bq deleted the fix/ethclient-test branch November 22, 2023 11:28
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