-
Notifications
You must be signed in to change notification settings - Fork 35
Fix out of sockets problem #99
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested. Keeps code from initializing too many sockets, even when the sockets aren't used initially.
I'm not clear why only socket #0 can be used for DNS. There's a use case where sockets use IP address rather than host names, and this reduces the available sockets. I don't know whether it's better to reserve a DNS socket, or raise an exception if the application uses too many sockets (like other uses of sockets).
edit: I don't see the linkage that enforces DNS to use socket #0. In a test, it seems to be fine with other sockets:
*** Opening socket 3
* Opening W5k Socket, protocol=2
* DNS: Sending request packet...
* socket_available called on socket 3, protocol 2
If that's the case, then I lean toward allowing the user code to use all available sockets, and raise an exception if it uses too many (for whatever purpose).
P.S. raw socket I think is needed for ping
, but I don't see that ping
is implemented in this library
Good question. My thought process was:
So, by leaving one socket free, these other calls will succeed.
Currently DHCP and DNS modules both import If a call is from anything other than a This PR works with the current version of the Wiznet5k library, but is written with a future versioning mind, where DHCP and DNS make direct calls to the Wiznet hardware instead of instantiating a I'm fully open to just allowing all the hardware sockets to instances of Also, this PR might be shelved as a draft until the updates to DHCP and DNS have been merged. |
I forgot about DHCP, which I think is also UDP, though I never understood why it's needed at the user level when all of the other CircuitPython networking environments don't. Somehow, DHCP doesn't seem to cut into the socket count in other environments either, but I don't know how that could be? You have a lot planned, you may want to involve @brentru, or someone else with more knowledge of this library than me. I'm always happy to test, but not the best one for code reviews |
If the WIZNET5K instance has The DHCP protocol doesn't take any action until the @brentru does pop in every now and then. I greatly appreciate your comments and testing. Thank you 😃 |
BTW, I do think that the assumption that there will be a |
# Conflicts: # adafruit_wiznet5k/adafruit_wiznet5k_socket.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I tested successfully with the simpleserver / client and the simpletest that uses requests library.
Thank you for this fix @BiffoBear!
Updating https://github.com/adafruit/Adafruit_CircuitPython_VL53L1X to 1.1.10 from 1.1.9: > Merge pull request adafruit/Adafruit_CircuitPython_VL53L1X#15 from jposada202020/adding_version_metadata > Add upload url to release action Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 2.4.0 from 2.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#97 from BiffoBear/shorter_socket_exit_timeout > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#99 from BiffoBear/fix_out_of_sockets > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#101 from BiffoBear/fix_socket.close()_behaviour Updating https://github.com/adafruit/Adafruit_CircuitPython_HID to 5.3.4 from 5.3.3: > Merge pull request adafruit/Adafruit_CircuitPython_HID#113 from dhalbert/fix-led-status > Add upload url to release action > Add .venv to .gitignore > Update .pylintrc for v2.15.5 > Fix release CI files > Update pylint to 2.15.5 > Updated pylint version to 2.13.0 > Switching to composite actions Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
closes #93 Implements a method of tracking which Wiznet hardware sockets are assigned to instances of
socket.socket()
.This works best when the user creates a
socket.socket()
instance using thewith
statement.If the socket is created using
x = socket.socket()
the user must callsocket.close()
to be sure that the hardware socket will be released.When garbage collection occurs,
socket.__del__()
will release the socket, but this is not reliable.