Close all and counts #13

justmobilize · 2024-04-25T21:47:59Z

Added to properties: open_sockets and freeable_open_sockets as informational.
Added connection_manager_close_all which will close out all open sockets to free resources

fixes #7

justmobilize · 2024-04-25T21:51:11Z

examples/connectionmanager_helpers.py

@@ -24,14 +24,38 @@

 # get request session
 requests = adafruit_requests.Session(pool, ssl_context)
+connection_manager = adafruit_connection_manager.get_connection_manager(pool)


The output of this is so:

---------------------------------------- Nothing yet opened Open Sockets: 0 Freeable Open Sockets: 0 ---------------------------------------- Fetching from http://wifitest.adafruit.com/testwifi/index.html in a context handler Text Response This is a test of Adafruit WiFi! If you can read this, its working :) ---------------------------------------- 1 request, opened and freed Open Sockets: 1 Freeable Open Sockets: 1 ---------------------------------------- Fetching from http://wifitest.adafruit.com/testwifi/index.html not in a context handler ---------------------------------------- 1 request, opened but not freed Open Sockets: 1 Freeable Open Sockets: 0 ---------------------------------------- Closing everything in the pool ---------------------------------------- Everything closed Open Sockets: 0 Freeable Open Sockets: 0

adafruit_connection_manager.py

justmobilize · 2024-04-25T21:55:24Z

@RetiredWizard is this what you are looking for? Instead of calling:

adafruit_connection_manager._global_socketpool.clear()

you would call:

adafruit_connection_manager.connection_manager_close_all()

RetiredWizard · 2024-04-25T22:01:15Z

The new call feels much better than utilizing the private function call. I'll try it with my code when I get a chance although I'm sure you tested against my test code. 😁

justmobilize · 2024-04-25T22:19:13Z

This also specifically closes any open sockets and frees up as much as it can.

justmobilize · 2024-04-26T00:13:02Z

@anecdata would enjoy your feedback here. And if you think there's more extra info we could give to the advanced developer

justmobilize · 2024-04-26T00:14:19Z

@DJDevon3, I updated the example to show counts (made public methods for getting them) for using with. Thoughts?

justmobilize · 2024-04-26T17:38:01Z

@RetiredWizard let me know what your testing finds!

RetiredWizard · 2024-04-26T22:54:07Z

I'll try to set up some testing later tonight....

RetiredWizard · 2024-04-27T00:52:13Z

So I've been poking around trying to figure out where this is coming from but after changing my reset code block as follows:

adafruit_connection_manager.connection_manager_close_all()
radio.disconnect()
radio = None
esp32_cs.deinit()
esp32_ready.deinit()
esp32_reset.deinit()
spi.deinit()
requests = None
pool = None

The second attempt to make a connection/request results in the following:

Connected: False
3
10.0.0.197
Connected: False
3
10.0.0.197
esp32_ready: False
Traceback (most recent call last):
  File "<stdin>", line 51, in <module>
  File "adafruit_requests.py", line 591, in get
  File "adafruit_requests.py", line 525, in request
  File "/lib/adafruit_connection_manager.py", line 266, in get_socket
  File "adafruit_esp32spi/adafruit_esp32spi_socket.py", line 44, in getaddrinfo
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 666, in get_host_by_name
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 340, in _send_command_get_response
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 247, in _send_command
  File "adafruit_esp32spi/adafruit_esp32spi.py", line 200, in _wait_for_ready
ValueError: Object has been deinitialized and can no longer be used. Create a new object.

I've traced it to the esp32_ready pin. It appears that adafruit_esp32spi is attempting to access the original esp32_ready object that was deinit in the reset block rather than the new esp32_ready object created in the second connect/request block. I'm trying to figure out how the original instance is being retained but don't really know how connection manager is working well enough yet.

The line causing the error is the last line of the test code snippet:

response = requests.get(text_url,headers=None,timeout=15000)

RetiredWizard · 2024-04-27T01:06:51Z

Actually, given the debug print statements in my test code, I'm thinking that I was having a similar issue with esp32_ready not being a valid object for the second requests.get call when I originally found the issue and before finding the adafruit_connection_manager._global_socketpool.clear() method.

justmobilize · 2024-04-27T01:35:54Z

Hmmm, so it seems like we may need 2 options:

Close everything but keep the radio, pool and SSL active
Close everything AND kill and kill the tracked radio, pool and SSL.

I think the hard part on 2 is that if you haven't fully released everything and still have one of those objects referenced somewhere that it will cause problems...

@dhalbert do you have thoughts? Should CM assume you have de-inited everything if you call this? And if you didn't, create new pools SSL contexts and everything?

RetiredWizard · 2024-04-27T01:48:12Z

I'm still just scratching at the surface, but the old method adafruit_connection_manager._global_socketpool.clear() worked. I'm wondering if the _free_sockets method is closing the sockets but retaining the socket data so when a closed socket is reopened it grabs all the old objects.

        """Get a new socket and connect"""
        if session_id:
            session_id = str(session_id)
        key = (host, port, proto, session_id)
        if key in self._open_sockets:
            socket = self._open_sockets[key]
            if self._available_socket[socket]:
                self._available_socket[socket] = False
                return socket

This code from get_socket looks to me like if I try and open a new socket for the same URL it will simply reopen the old socket since the _free_sockets method didn't actually clear the data, only flag the socket as being closed/free.

Edit: Maybe that's the distinction between 1 and 2 you were talking about above....

RetiredWizard · 2024-04-27T02:05:00Z

I think the hard part on 2 is that if you haven't fully released everything and still have one of those objects referenced somewhere that it will cause problems...

I don't think close/deinit gets used very often, if someone does call a close, I think it's reasonable to expect them to re-initialize the associated resources if they need to reopen.

I guess I don't do enough WiFi to understand what advantage there would be in retaining the radio, pool and SSL after a close.

justmobilize · 2024-04-27T03:21:52Z

@RetiredWizard so I think adding an option to release references to the radio, pool and SSL make sense.

My worry would be that people do it without realizing what is happening and thus keep creating new things that aren't fully de-inited and use up all the memory on having 100 pools...

I'll add another optional param and let you know when it's ready for re-test

justmobilize · 2024-04-27T04:55:48Z

@RetiredWizard I added another option. This should work:

adafruit_connection_manager.connection_manager_close_all(release_references=True)

This will close sockets and clear out:

adafruit_connection_manager._global_connection_managers
adafruit_connection_manager._global_ssl_contexts
adafruit_connection_manager._global_socketpools

So as long as you've fully release the objects with you:

radio.disconnect()
radio = None
esp32_cs.deinit()
esp32_ready.deinit()
esp32_reset.deinit()
spi.deinit()

before trying to add it again, you should be good.

RetiredWizard · 2024-04-27T15:12:17Z

Seems to work great 😁 Thanks! I did add the following block before calling the cloase_all:

            if self.response:
                self.response.close()
            self.response = None

But that's probably not connection_manager's responsibility. Maybe a note in the API documentation to close out any returned requests before shutting down the connection_manager. but then that may be getting too application specific for API docs....

Edit: I tested on an ESP32SPI (Arduino Nano RP2040 Connect), PicoW and an ESP32S3

justmobilize · 2024-04-27T15:50:02Z

@RetiredWizard awesome! Response.close() just does two things:

Closes the socket
Sets the socket to None

So yes, if you pass a response around and the socket has been closed and not read, you'll get errors. But if one is intentionally trying to close everything, hopefully they would guard against that.

Thank you for all your testing!

justmobilize · 2024-04-27T15:50:49Z

@dhalbert would love a review when you have a chance!

dhalbert

Hi, several questions and suggestions.

adafruit_connection_manager.py

justmobilize · 2024-04-27T22:42:03Z

@dhalbert updated per our conversation!

dhalbert

Thanks for the extended conversation! One suggestion. I think it is clearer now, and would be interested in comments from others as well.

adafruit_connection_manager.py

dhalbert · 2024-04-27T23:01:30Z

@RetiredWizard we'd appreciate your comments as well.

RetiredWizard

I looked through the code and it looks good to me, but I don't really have a strong understanding of how pools/sockets work so I'm not sure how much value that is for you. I did make a suggestion regarding the retry of the _get_connected_socket call but it's not critical and would probably only have any value for esp32SPI boards.

RetiredWizard · 2024-04-27T23:59:18Z

adafruit_connection_manager.py

+                    addr_info, host, port, timeout, is_ssl, ssl_context
+                )
+        if isinstance(result, Exception):
+            raise RuntimeError(f"Error connecting socket: {result}") from result


This block is a lot cleaner. I still worry about tossing the first error though, in my experience the first error is often the most important. What do you think about saving the result of the first _get_connect_socket call if it's an exception and then if the second call also returns an exception, printing the result before actually raising the error. Something like:

if isinstance(result, Exception): if result_sav is not None: print(f"Error connecting socket: {sys.exception(result_sav)}\nTrying again") raise RuntimeError(f"Error connecting socket: {result}") from result

This fakes the timing of events a bit since both errors would be displayed after both attempts fail, but has the advantage of keeping the output clean if the second attempt succeeds.

Edit: Actually if the results is an exception at the end then the first attempt must have failed so you wouldn't need to do the result_sav is not None test.

@RetiredWizard this is tough. I wouldn't want a core library printing. I think with a little thought, we could flag a debug mode that prints them all (maybe with a connection_manager_set_debug(socket). I also have a fear on too much bloat...

What about:

last_result = "" result = self._get_connected_socket( addr_info, host, port, timeout, is_ssl, ssl_context ) if isinstance(result, Exception): # Got an error, if there are any available sockets, free them and try again if self.available_socket_count: last_result = f", first error: {result}" self._free_sockets() result = self._get_connected_socket( addr_info, host, port, timeout, is_ssl, ssl_context ) if isinstance(result, Exception): raise RuntimeError(f"Error connecting socket: {result}{last_result}") from result

This would update this test:

def test_get_socket_runtime_error_ties_again_only_once(): mock_pool = mocket.MocketPool() mock_socket_1 = mocket.Mocket() mock_socket_2 = mocket.Mocket() mock_pool.socket.side_effect = [ mock_socket_1, RuntimeError("error 1"), RuntimeError("error 2"), RuntimeError("error 3"), mock_socket_2, ] free_sockets_mock = mock.Mock() connection_manager = adafruit_connection_manager.ConnectionManager(mock_pool) connection_manager._free_sockets = free_sockets_mock # get a socket and then mark as free socket = connection_manager.get_socket(mocket.MOCK_HOST_1, 80, "http:") assert socket == mock_socket_1 connection_manager.free_socket(socket) free_sockets_mock.assert_not_called() # try to get a socket that returns a RuntimeError twice with pytest.raises(RuntimeError) as context: connection_manager.get_socket(mocket.MOCK_HOST_2, 80, "http:") assert "Error connecting socket: error 2, first error: error 1" in str(context) free_sockets_mock.assert_called_once()

Yea If that does what it looks like it does, that's great. I also see the argument to drop for the sake of bloat though. Some of the WiFi boards have really thin available memory and a few bytes can make a big difference. I'll let you make the call 😁

@dhalbert are you good with this change?

this seems ok. You can save just save the exception (first_exception) and then only make a formatted string in the second if.

Yes, reducing code size is good wherever you can do it for frozen libraries.

justmobilize · 2024-04-28T02:03:30Z

adafruit_connection_manager.py

@@ -64,7 +64,7 @@ def connect(self, address: Tuple[str, int]) -> None:
        try:
            return self._socket.connect(address, self._mode)
        except RuntimeError as error:
-            raise OSError(errno.ENOMEM) from error
+            raise OSError(errno.ENOMEM, str(error)) from error


for ESP32SPI, a little more information

justmobilize · 2024-04-28T02:04:47Z

@dhalbert and @RetiredWizard ready for the last (I hope) review!

RetiredWizard · 2024-04-28T02:08:03Z

Looks good to me...

justmobilize · 2024-04-28T02:11:38Z

@RetiredWizard does this close out #14 as well?

RetiredWizard · 2024-04-28T02:25:11Z

Yes is does, thanks!

justmobilize · 2024-04-28T02:41:20Z

@dhalbert what are the next steps?

Close all and counts

ef4a49d

justmobilize commented Apr 25, 2024

View reviewed changes

justmobilize marked this pull request as ready for review April 25, 2024 21:53

Add release_references

dcd6be5

dhalbert requested changes Apr 27, 2024

View reviewed changes

adafruit_connection_manager.py Outdated Show resolved Hide resolved

adafruit_connection_manager.py Outdated Show resolved Hide resolved

Code review comments

9aa4b45

dhalbert requested changes Apr 27, 2024

View reviewed changes

adafruit_connection_manager.py Outdated Show resolved Hide resolved

rename var

f33d78c

justmobilize requested a review from dhalbert April 27, 2024 23:00

dhalbert approved these changes Apr 27, 2024

View reviewed changes

RetiredWizard approved these changes Apr 28, 2024

View reviewed changes

Little error cleanup

601ee66

justmobilize commented Apr 28, 2024

View reviewed changes

dhalbert approved these changes Apr 28, 2024

View reviewed changes

RetiredWizard mentioned this pull request Apr 28, 2024

Question regarding get_socket error reporting #14

Closed

dhalbert merged commit 1531496 into adafruit:main Apr 28, 2024
1 check passed

justmobilize deleted the close-all-and-counts branch April 28, 2024 04:30

Close all and counts #13

Close all and counts #13

Conversation

justmobilize commented Apr 25, 2024 • edited Loading

justmobilize Apr 25, 2024

Choose a reason for hiding this comment

justmobilize commented Apr 25, 2024

RetiredWizard commented Apr 25, 2024

justmobilize commented Apr 25, 2024

justmobilize commented Apr 26, 2024

justmobilize commented Apr 26, 2024

justmobilize commented Apr 26, 2024

RetiredWizard commented Apr 26, 2024

RetiredWizard commented Apr 27, 2024 • edited Loading

RetiredWizard commented Apr 27, 2024

justmobilize commented Apr 27, 2024

RetiredWizard commented Apr 27, 2024 • edited Loading

RetiredWizard commented Apr 27, 2024 • edited Loading

justmobilize commented Apr 27, 2024

justmobilize commented Apr 27, 2024

RetiredWizard commented Apr 27, 2024 • edited Loading

justmobilize commented Apr 27, 2024

justmobilize commented Apr 27, 2024

dhalbert left a comment

Choose a reason for hiding this comment

justmobilize commented Apr 27, 2024

dhalbert left a comment

Choose a reason for hiding this comment

dhalbert commented Apr 27, 2024

RetiredWizard left a comment

Choose a reason for hiding this comment

RetiredWizard Apr 27, 2024 • edited Loading

Choose a reason for hiding this comment

justmobilize Apr 28, 2024

Choose a reason for hiding this comment

justmobilize Apr 28, 2024

Choose a reason for hiding this comment

RetiredWizard Apr 28, 2024

Choose a reason for hiding this comment

justmobilize Apr 28, 2024

Choose a reason for hiding this comment

dhalbert Apr 28, 2024

Choose a reason for hiding this comment

justmobilize Apr 28, 2024

Choose a reason for hiding this comment

justmobilize commented Apr 28, 2024

RetiredWizard commented Apr 28, 2024

justmobilize commented Apr 28, 2024

RetiredWizard commented Apr 28, 2024

justmobilize commented Apr 28, 2024

justmobilize commented Apr 25, 2024 •

edited

Loading

RetiredWizard commented Apr 27, 2024 •

edited

Loading

RetiredWizard commented Apr 27, 2024 •

edited

Loading

RetiredWizard commented Apr 27, 2024 •

edited

Loading

RetiredWizard commented Apr 27, 2024 •

edited

Loading

RetiredWizard Apr 27, 2024 •

edited

Loading