Skip to content

Conversation

jfurcean
Copy link
Contributor

@jfurcean jfurcean commented Feb 17, 2021

Fixes problem when there are free sockets but are not closed when needed. See #73

@jfurcean jfurcean changed the title x out of sockets issue fix out of sockets issue Feb 17, 2021
@jfurcean jfurcean force-pushed the close_free_sockets branch 2 times, most recently from 52e4c0c to 57bdb5e Compare February 17, 2021 04:14
@brentru brentru requested a review from a team February 17, 2021 15:41
@brentru
Copy link
Member

brentru commented Feb 17, 2021

@jfurcean Could you clarify why you are except'ing the RuntimeError: Out of sockets"? Is this being thrown erroneously by CircuitPython?

@Neradoc
Copy link
Contributor

Neradoc commented Feb 17, 2021

The assumption is that it is being thrown correctly, so it triggers a retry on the loop, which in turn causes a call to self._free_sockets() which would close the "free" sockets. Requests otherwise doesn't automatically close the sockets it uses, it keeps them, only marking them as "free" when no longer in use, and reuses them when you access the same host/port, leading to inevitably running out of sockets.

In 6.2 beta 1 the same code would cause a MemoryError on sock.connect() in that same loop which in turn would also cause the freeing of sockets. That change to a more specific error message is a consequence of the "Separate SSLSocket from Socket" PR.

@brentru
Copy link
Member

brentru commented Feb 17, 2021

In 6.2 beta 1 the same code would cause a MemoryError on sock.connect() in that same loop which in turn would also cause the freeing of sockets. That change to a more specific error message is a consequence of the "Separate SSLSocket from Socket" PR.

I see, thank you for the clarification! I'm going to implement a similar patch within MiniMQTT.

@brentru brentru merged commit f08e339 into adafruit:master Feb 17, 2021
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants