Connection handlers can be called multiple times with different signatures

[rgov]

A pattern that appears in the codebase is to try calling a (dis)connection handler with one set of arguments, and if it raises a TypeError, call again with a different set:

python-socketio/src/socketio/server.py

Lines 537 to 542 in b01b197

	try:
	success = self._trigger_event(
	'connect', namespace, sid, self.environ[eio_sid])
	except TypeError:
	success = self._trigger_event(
	'connect', namespace, sid, self.environ[eio_sid], None)

This is risky if the event handler itself can raise a TypeError, unrelated to the handler's arity. This could cause the handler to be run up to the point it throws a TypeError, then the TypeError is consumed and the handler is called again.

A contrived example might be:

@sio.on('connect', namespace='*')
async def on_client_connect(namespace, sid, environ, *args):
    initialize_client_state(sid)
    2 + "two"

This will be called once with (namespace, sid, environ) and initialize some client state. Then it will raise a TypeError, so it will be called again with (namespace, sid, environ, auth). The client state will be initialized a second time, which might lead to an unexpected state, resource leaks, etc.

It may be better to check inspect.signature(handler).parameters -- however when the event is triggered with self._trigger_event() it doesn't have a reference to the handler callable.

(I'm filing this only because while debugging some other part of my application, I kept getting complaints about including auth or not including auth in my parameter list. Finally I settled on adding *args and moving on.)

[miguelgrinberg]

Yes, I am aware that there is a risk. I decided I could live with it, because it is highly unlikely that you will write a handler that can accept both function signatures. It is going to be either one or the other, so the risk of the handler being called twice is extremely low in my opinion.

It may be better to check inspect.signature(handler).parameters

Yes, but introspection is very expensive. The design that I used is intended to be more performant. The most common of the signatures is called first, without doing any checking. This is the path that most applications will take, without any performance penalties. Adding introspection in every handler invocation is going to cause a significant performance degradation.

I kept getting complaints about including auth or not including auth in my parameter list.

You should have auth in your handler. This is in line with the most recent Socket.IO specification. If you get an error saying that the handler was called without auth that means that you had an unrelated TypeError in your handler. So in that case the correct solution is to address the type error. The function signature should not be changed.

[rgov]

Right, I did have an unrelated TypeError which was probably using with x instead of async with x somewhere in the body.

The parameter introspection result could be memoized, or recorded at the time the handler is attached with .on().

A minimal fix/workaround/improvement is to try calling the handler with 4 arguments first. If that raises a TypeError, it means either (a) the handler actually takes only 3 arguments, in which case we can safely fall back, or (b) the handler itself has a bug. In the buggy 4-argument case, the second call (with 3 arguments) will immediately fail with a TypeError from the wrong argument count, without re-executing any part of the handler body.

[miguelgrinberg]

@rgov Based on your experience I think a small improvement would be something like this: if the first call raises a TypeError, save the exception, and then attempt to call the handler with the second signature. If the second call fails with TypeError, then raise the saved exception instead of the new one, since in this case the most likely situation is that the first call failed with an unrelated TypeError. This ensures that for people that use the most modern signature the error is accurate, at the expense of the people who use the old signature, which will still get a TypeError exception, but in the wrong line and for the wrong reason.

I'm also open to consider a caching solution with introspection, or maybe just looking at the details of the TypeError exception to determine if it is caused by a missing argument or something else.