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

#7297 Add tests for closed keepalive connections #7308

Closed
wants to merge 2 commits into from

Conversation

filip-muller
Copy link

What do these changes do?

Tests for the bug discussed in #7297 (ClientSession tries to reuse a closed keepalive connection). The new tests are not passing due to the bug. Interestingly, the one with asyncio.sleep passes, as the connection_lost gets called before the request is made and a new connection is established. The one with time.sleep doesn't, connection_lost doesn't get called "early enough", nor is transport.is_closing() True.

Are there changes in behavior for the user?

No

Related issue number

#7297

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@Dreamsorcerer
Copy link
Member

Hmm, that's a little odd. The test fails with SystemError: AST constructor recursion depth mismatch (before=117, after=173), whatever that means...

@filip-muller
Copy link
Author

That's strange, I got a ServerDisconnectedError on Ubuntu with Python 3.10.6. Can you try to run the Python 3.10 test?

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Jul 8, 2023

Yeah, the 3.10 test seems to get past there. Just happens with the 3.11 test.

I get a pytest issue reopened about it, but no movement on it since. pytest-dev/pytest#10763

@Dreamsorcerer
Copy link
Member

Merged into #7363.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:skip This PR does not need to include a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants