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
Consistent error value of context timeout on subscription Fetch() interface. #1011
Conversation
e1208d3
to
009889b
Compare
009889b
to
0834239
Compare
js_test.go
Outdated
t.Fatalf("Expect ErrTimeout, got err=%#v", err) | ||
} | ||
}) | ||
t.Run("Context timeout should return context error", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test case already covered here: https://github.com/nats-io/nats.go/blob/main/test/js_test.go#L6719-L6726
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind, I see now that this test is the one that has the coverage for the issue. In the one I mentioned the context has not yet timed out but here it has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
js_test.go
Outdated
@@ -939,6 +941,21 @@ func TestJetStreamExpiredPullRequests(t *testing.T) { | |||
t.Fatalf("Expected error and wait for 250ms, got err=%v and dur=%v", err, dur) | |||
} | |||
} | |||
|
|||
t.Run("MaxWait timeout should return nats error", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move these test blocks to be as part of this other test? https://github.com/nats-io/nats.go/blob/main/test/js_test.go#L6869
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! Just one request to move the tests to be under test/js_test.go under the TestJetStreamPullSubscribeFetchContext
tests.
0834239
to
74018d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Follow TDD principle, write test watch it fails.
If user provides context object and context timeout, always return the timeout error from context.
74018d1
to
37edc5e
Compare
I accidentally leaved an empty line in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
When using Subscription.Fetch() with
nats.Context(ctx)
option,if context timeout receiving any message,
Fetch
will return error.There are two possible value of error here,
nats.ErrTimeout
andcontext.DeadlineExceeded
, based on some timing in the process/gorouting. If context timeout before subscription object trying to fetch message,nats.ErrTimeout
is returned. Otherwise,context.DeadlineExceeded
is returned.This PR make the error value of fetch timeout predicable, if Fetch is called with
nats.Context(ctx)
, timeout always returncontext.DeadlineExceeded
; if called withnats.MaxWait(timeout)
, timeout always returnnats.ErrTimeout
.