fix: better handle canceled contexts in queries #5
Conversation
Stebalien
commented
Jul 16, 2021
Stebalien
commented
- Make sure we don't block forever writing the result.
- Make sure we stop the query when the context is canceled.
- Allow receiving one result after the context is canceled, the query could have been writing it and may not have checked if the context was canceled yet.
blockstore_test.go
Outdated
| received := 0 | ||
| for range ch { | ||
| received++ | ||
| require.LessOrEqual(t, received, 1, "expected to receive at most one result after the context was canceled") |
There was a problem hiding this comment.
Thats a very strange set of semantics to expect here. I guess it makes sense, but is this really what we want the blockstore interface to guarantee?
There was a problem hiding this comment.
No, we don't really have any guarantees. The blockstore is allowed to buffer responses.
There was a problem hiding this comment.
I.e., it could give us 10 items. I guess I can just make the test allow that to be more flexible?
There was a problem hiding this comment.
Certainly not specified by the spec, but I assume this is only testing one blockstore implementation, so you can make more assumptions :)
| case ret <- cid.NewCidV1(codec, mh): | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| case ctx.Err() != nil: |
There was a problem hiding this comment.
IMO this ctx check is now kinda redundant, as it's surrounded by another two "are we cancelled" checks.
There was a problem hiding this comment.
And in turn you could also replace the naked switch with just a good old if err != nil check.
There was a problem hiding this comment.
IMO this ctx check is now kinda redundant, as it's surrounded by another two "are we cancelled" checks.
It's to avoid logging in that case. But I agree.
blockstore_test.go
Outdated
| received := 0 | ||
| for range ch { | ||
| received++ | ||
| require.LessOrEqual(t, received, 1, "expected to receive at most one result after the context was canceled") |
There was a problem hiding this comment.
Certainly not specified by the spec, but I assume this is only testing one blockstore implementation, so you can make more assumptions :)
I agree. I'll make it a little more flexible. |
1. Make sure we don't block forever writing the result. 2. Make sure we stop the query when the context is canceled. 3. Allow receiving a few results after the context is canceled, the query could have been writing it and may not have checked if the context was canceled yet.
Stebalien
force-pushed
the
fix/context-cancel-query
branch
from
0c5a316 to
3823254
Compare
|
Ok, I gave it a flex of "10". We need to cancel within 10 results. |
