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 ssl_sock is closed if assert_fingerprint or match_hostname fail #2992

Merged

Conversation

graingert
Copy link
Contributor

@graingert graingert commented Apr 25, 2023

Closes #2991
Closes #2999

extracted from #2842

this doesn't need a backport to v1.26 because the issue was introduced in ea96fde

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @graingert for finding and fixing this!

@sethmlarson sethmlarson merged commit 4fb8da2 into urllib3:main Apr 29, 2023
93 of 97 checks passed
@graingert graingert deleted the fix-ssl-wrap-socket-and-match-hostname-rw branch April 29, 2023 19:03
@pquentin
Copy link
Member

This does fix the error nicely, and I did not initially understand why. But @graingert explained on Discord that "The problem is they are using a single threaded server and the socket is being kept open by a reference in the traceback". That was still cryptic to me, so here's an attempt on a more detailed explanation.

  • When _assert_fingerprint or _match_hostname fail, they raise an exception. A traceback is attached to that exception, and the traceback keeps a reference to the socket.
  • urllib3 keeps a reference to that exception to include it in the MaxRetryError exception it will eventually raise. This means the garbage collector will not close the socket on its own, which means the connection will be kept open.
  • pytest-httpserver and pytest-localserver are single threaded and can only accept one request at a time. So when urllib3 retries, it won't be able to open a connection, because the server is waiting for the previous try to finish. That's a deadlock!

When we disable retries, the situation is similar. The test passes, but I think pytest somehow keeps a reference to the exception, which means that the server cannot shutdown, so its fixture never finishes, so the test suite hangs too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants