del Client.instances[self.id] Key error

[CVxTz]

Description

Hello !

I had this error:

 Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/nicegui/background_tasks.py", line 52, in _handle_task_result
    task.result()
  File "/usr/local/lib/python3.10/site-packages/nicegui/client.py", line 259, in handle_disconnect
    self.delete()
  File "/usr/local/lib/python3.10/site-packages/nicegui/client.py", line 318, in delete
    del Client.instances[self.id]
KeyError: '51d921eb-0045-4f76-9b2e-ec8e3021e24c' 

This was also mentioned in #1826

@falkoschindler mentioned the absence of environ as the reason why prune_clients would not delete the same clients as handle_disconnect. But clients that connected can still get environ as None (for whatever reason) as in the socker-io library we have this function def:

    def get_environ(self, sid, namespace=None):
        """Return the WSGI environ dictionary for a client.

        :param sid: The session of the client.
        :param namespace: The Socket.IO namespace. If this argument is omitted
                          the default namespace is used.
        """
        eio_sid = self.manager.eio_sid_from_sid(sid, namespace or '/')
        return self.environ.get(eio_sid)

Which can return None.

I see multiple options:

• On client connect, verify that environ does not end up being None anyway:

  async def _on_handshake(sid: str, data: Dict[str, str]) -> bool:
     client = Client.instances.get(data['client_id'])
     if not client:
         return False
     client.environ = sio.get_environ(sid)  # Add some check or default non null value here
     client.tab_id = data['tab_id']
     await sio.enter_room(sid, client.id)
     client.handle_handshake()
     return True

• On .delete(), verify that the id exists and was not already deleted by prune_clients
• Understand why sio.get_environ might return None and fix it.

I can't really reproduce the issue so I am not sure of what I am saying but I think its worth looking into this.

[falkoschindler]

Thanks for the new input on this matter, @CVxTz!

The fact that sio.get_environ might return None could indeed cause some false assumption, even though I'm still struggling to get my head around the exact scenario where this could happen. Maybe there was a problem creating the socket connection, or it has been closed already. Adding an early return False for this case might be a possible solution. But first I'd like to find some kind of reproduction in order to verify the solution.

[rodja]

I wonder if we could get rid of relying on sio.get_environ so much:

• in nicegui.py we use it in _on_disconnect to find the client id, but we could also use sio's user_session to save the client id directly on connection
• in client.py the has_socket_connection can be implemented with the new client.tab_id property
• in client.py the ip property is already implemented without relying on client.environ in our 2.0 branch (see provide client.request and make client.ip available even before client.connected (breaking change) #2399)

While digging through the code I found a possible scenario where client.environ is None: if a disconnect happens before the handshake could be completed. Does anyone has an idea on how to provoke this for verification?

[ZAn-next]

I have got the same error when I use storage.client. I am not sure if there is any memory leakage as well.
Just be curious: Do we consider prioritizing merging the change to v1.x, instead of v2.0?

[falkoschindler]

@ZAn-next We just had the idea of replacing environ with tab_id for checking if a client has_socket_connection: 200dea9. This might have solved this issue. Oh and GitHub already closed it because of my commit message. Let's see if the problem is gone and re-open it otherwise.