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

fix: keep waiting if connection is not ready to close #1636

Closed

Conversation

Yangruipis
Copy link

related to #111

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 17, 2022

I need more information here...

@Kludex Kludex added the waiting author Waiting for author's reply label Sep 17, 2022
@Yangruipis
Copy link
Author

Here is the way to reproduce this problem.

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 18, 2022

If you can explain the problem, and the solution, it would save me time, and I'd really appreciate it.

@Yangruipis
Copy link
Author

Yangruipis commented Oct 4, 2022

If you can explain the problem, and the solution, it would save me time, and I'd really appreciate it.

Sorry for my late response.

Problem

The close of the connection after timeout_keep_alive may raise error like that:

h11._util.LocalProtocolError: can't handle event type ConnectionClosed when role=SERVER and state=SEND_RESPONSE

Because the function timeout_keep_alive_handler is called when the connection is still sending data. This may happen with a very little probability under high concurrency, like a locust test.

def timeout_keep_alive_handler(self) -> None:
"""
Called on a keep-alive connection if no new data is received after a short
delay.
"""
if not self.transport.is_closing():
event = h11.ConnectionClosed()
self.conn.send(event)
self.transport.close()

Solution

  1. if the connection is still sending data, do not close it
  2. (optional) put it back to the loop, which will be called after another timeout. Or we do nothing until another timeout_keep_alive_task is launched on response complete.

def timeout_keep_alive_handler(self) -> None:
"""
Called on a keep-alive connection if no new data is received after a short
delay.
"""
if not self.transport.is_closing() and self.cycle.response_complete:
event = h11.ConnectionClosed()
self.conn.send(event)
self.transport.close()
# keep waiting if not ready
elif not self.cycle.response_complete:
self.timeout_keep_alive_task = self.loop.call_later(
self.timeout_keep_alive, self.timeout_keep_alive_handler
)

@Kludex Kludex added this to the Version 0.20.0 milestone Oct 22, 2022
@Kludex Kludex mentioned this pull request Oct 28, 2022
13 tasks
@Kludex
Copy link
Sponsor Member

Kludex commented Oct 30, 2022

I cannot reproduce your example. Are you able to create a test case on this PR?

@Kludex Kludex modified the milestones: Version 0.20.0, Version 0.21.0 Oct 31, 2022
@Yangruipis
Copy link
Author

Yangruipis commented Oct 31, 2022

I cannot reproduce your example. Are you able to create a test case on this PR?

I have added a new case and some comments. But it's interesting that the new case passed locally, but failed in CI. I'll check my local package version firstly, and reopen this PR if I can make sure about it.

However, if you want to reproduce this issue, just run the new test case with the master branch code. It will raise error like that:

image

@Yangruipis Yangruipis closed this Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting author Waiting for author's reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants