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

Ensure HTTP/2 connections are gracefully closed on async cancellation #757

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

karpetrosyan
Copy link
Member

@karpetrosyan karpetrosyan commented Jul 14, 2023

Closes #756

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

TODO

  • Add failing tests
  • Gracefully handle cancellation for unstarted http/2 requests
  • Update changelog

@karpetrosyan karpetrosyan requested a review from a team July 14, 2023 09:22
@karpetrosyan karpetrosyan added bug Something isn't working tests Tests & coverage labels Jul 14, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@karpetrosyan karpetrosyan changed the title Handle failed HTTP/2 requests gracefully Ensure HTTP/2 connections are gracefully closed on cancellation Jul 15, 2023
Copy link
Contributor

@T-256 T-256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could (async )with self._state_lock: move under _state_housekeeping function?

@zanieb
Copy link
Contributor

zanieb commented Jul 15, 2023

could (async )with self._state_lock: move under _state_housekeeping function?

I agree, it's always used.

@karpetrosyan karpetrosyan mentioned this pull request Aug 1, 2023
1 task
@tomchristie
Copy link
Member

tomchristie commented Aug 1, 2023

At thanks! The failing test case is especially helpful here.

Pretty sure this resolution is overcomplicated, tho. I think we actually want here is?...

  1. This existing try...except needs to be expanded...

try:
kwargs = {"request": request}
async with Trace("send_connection_init", logger, request, kwargs):
await self._send_connection_init(**kwargs)
except BaseException as exc:
with AsyncShieldCancellation():
await self.aclose()
raise exc

...so that it instead covers all of this block...

try:
kwargs = {"request": request}
async with Trace("send_connection_init", logger, request, kwargs):
await self._send_connection_init(**kwargs)
except BaseException as exc:
with AsyncShieldCancellation():
await self.aclose()
raise exc
self._sent_connection_init = True
# Initially start with just 1 until the remote server provides
# its max_concurrent_streams value
self._max_streams = 1
local_settings_max_streams = (
self._h2_state.local_settings.max_concurrent_streams
)
self._max_streams_semaphore = AsyncSemaphore(local_settings_max_streams)
for _ in range(local_settings_max_streams - self._max_streams):
await self._max_streams_semaphore.acquire()

ie. the except case to move down to line 127 there.

  1. This call to acquire()...

await self._max_streams_semaphore.acquire()

Should also be surrounded with...

try:
    await self._max_streams_semaphore.acquire()
 except BaseException as exc: 
     with AsyncShieldCancellation(): 
         await self.aclose() 
     raise exc

We could actually avoid (1) if we instead addressed the refactoring suggested in #738 or refactored self._send_connection_init(**kwargs) so that our initial handshake waits for the settings response before we instantiate our max_streams_semaphore. But let's not be getting into that just now.

@tomchristie
Copy link
Member

Ugh. I need to think about this one more clearly, it's making my brain mush.

@tomchristie tomchristie changed the title Ensure HTTP/2 connections are gracefully closed on cancellation Ensure HTTP/2 connections are gracefully closed on async cancellation Aug 3, 2023
@karpetrosyan
Copy link
Member Author

I would recommend using this stupid solution for the time being and not waiting for http/2 refactoring from #770

@karpetrosyan karpetrosyan requested a review from a team August 28, 2023 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tests Tests & coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure HTTP/2 connections are gracefully closed on async cancellation
4 participants