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

Fetch ignores context timeout. #834

Closed
andreib1 opened this issue Sep 29, 2021 · 2 comments · Fixed by #838
Closed

Fetch ignores context timeout. #834

andreib1 opened this issue Sep 29, 2021 · 2 comments · Fixed by #838
Assignees

Comments

@andreib1
Copy link

andreib1 commented Sep 29, 2021

nats.go version: 1.13.3
nats version: 2.6.1

I have examined at the Fetch code in js.go and I think there may be some issues with the logic:

When a context is passed in that has a context timeout, and no explicit PullOpts timeout, the code sets the internal ttl to the default timeout value. It uses this default value ttl to time out the request returning context.DeadlineExceeded thereby ignoring the context timeout value.

When a timeout value is provided through PullOpts without a context, the timeout value is respected.

The check at the start of the function prevents supplying both, so therefore it is therefore impossible to provide a cancellable context to Fetch, and and also a timeout.

Providing no timeout value is causing Fetch to stall for us, but I also need to be able to cancel the context during a graceful shutdown of the server.

@andreib1
Copy link
Author

andreib1 commented Sep 29, 2021

At the point of testing the following:

if ttl < 0 {
  // At this point consider that we have timed-out
  return context.DeadlineExceeded
}

Maybe I'm incorrect, but a context has already been assured with a timeout value. If that's right, shouldn't this just check for the context being invalidated instead of ttl?

@wallyqs
Copy link
Member

wallyqs commented Oct 5, 2021

@andreib1 thanks again for the report! This is now fixed in the main branch.

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 a pull request may close this issue.

2 participants