-
Notifications
You must be signed in to change notification settings - Fork 263
Fix PullSubscription.fetch hang due to orphan lingering request #934
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1265,9 +1265,29 @@ async def _fetch_n( | |
|
|
||
| # Second request: lingering request that will block until new messages | ||
| # are made available and delivered to the client. | ||
| # | ||
| # Use the *remaining* deadline as the request's expires rather than | ||
| # the original full timeout. The original expires was computed at | ||
| # the very start of fetch() and may be nearly exhausted by the time | ||
| # we reach this point (e.g. when the server's 408 for the no-wait | ||
| # probe arrives just before the asyncio timer fires). Sending a | ||
| # lingering request with the full original expires in that situation | ||
| # creates an orphaned pull request that survives on the server long | ||
| # after the client has timed out, capturing the next published | ||
| # message and causing the subsequent fetch() to stall for the full | ||
| # timeout window. | ||
| deadline = JetStreamContext._time_until(timeout, start_time) | ||
| if deadline is not None and deadline <= 0: | ||
| raise asyncio.TimeoutError | ||
|
|
||
| next_req = {} | ||
| next_req["batch"] = needed | ||
| if expires: | ||
| if deadline is not None: | ||
| remaining_expires = int(deadline * 1_000_000_000) - 100_000 | ||
| if remaining_expires <= 0: | ||
| raise asyncio.TimeoutError | ||
| next_req["expires"] = remaining_expires | ||
| elif expires: | ||
| next_req["expires"] = expires | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The However, |
||
| if heartbeat: | ||
| next_req["idle_heartbeat"] = int(heartbeat * 1_000_000_000) # to nanoseconds | ||
|
|
@@ -1335,6 +1355,13 @@ async def _fetch_n( | |
| if JetStreamContext._is_heartbeat(status): | ||
| got_any_response = True | ||
| continue | ||
| if status in ( | ||
| api.StatusCode.NO_MESSAGES, | ||
| api.StatusCode.REQUEST_TIMEOUT, | ||
| ): | ||
| # No more messages will be delivered on this pull | ||
| # request; return what we have. | ||
| break | ||
| if JetStreamContext._is_processable_msg(status, msg): | ||
| needed -= 1 | ||
| msgs.append(msg) | ||
|
|
||
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.
This might also be worth discussing. This and line 1296 below raise
asyncio.TimeoutError. This is consistent with line 1234 above. However I question whether this is the right behavior. There's alsoFetchTimeoutError. So the current code is consistent, but is it consistently wrong? Should it beFetchTimeoutError? And fix line 1234 as well?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.
FYI we just upgraded to 2.15.0 in production and we're hitting many
asyncio.TimeoutErrorthat are uncaught because we expectnats.errors.TimeoutErrorinstead. We have to revert to2.14.0because of this since we can't update all our applications to catch this new Exception.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.
Tentative fix: #1008