feat(runtime-sdk): Add serialize option in WSGI entrypoint

[ryanking13]

As we discussed internally, this adds a serialize option to wsgi handler so that users can serialize each request if there applications are not async safe.

Originally, I thought we should add this to asgiref.sync.sync_to_async, but I realized that WSGI applications don't use asgiref.sync.sync_to_async at all, and it is for using synchronous ORMs inside async handlers in ASGI applications. So I ended up locking the entire request.

We can probably add the same option to asgi.py as well, but asgi.py is a bit more compliated due to lifespans, so I would like to discuss and land this to wsgi first.

[ask-bonk]

LGTM!

github run

[hoodmane]

Why do we unconditionally call defer_release()? I guess the idea of the context manager is to release the lock on all the unsuccessful paths, but the success path will release it via the on_close method.

[hoodmane]

Maybe could use a comment.

[hoodmane]

Don't we need to set this back to True at some point?

[hoodmane]

Okay I get it, RequestLock is only used once.

[hoodmane]

Perhaps this should be called MaybeRequestLock? We could get rid of all the if self._enabled: guards by doing:

    def __new__(cls, enabled: bool):
        if enabled:
            return object.__new__(cls)
        return contextlib.nullcontext()

[hoodmane]

Slightly confusing here with the nested try/finallys. So what we do is:

1. always call _close_iterable(), environ["wsgi.input"].close(), and on_close() in that order independently of which ones raise.
2. Errors in environ["wsgi.input"].close() are always suppressed (except XCPU).
3. Errors in _close_iterable() and on_close() propagate up the step
4. If both _close_iterable() and on_close() raise, the error from on_close() wins.

[hoodmane]

It occurs to me that one general concern is if we have:

try:
    do_something()
finally:
    cleanup()

it's possible that do_something() is stopped by an exceeded cpu error, and then cleanup() raises a different error replacing it, which gets caught and we lose track of the fact that we were supposed to stop early. This isn't really a problem introduced by this PR of course and you'd have to replace it with something awful like:

run_finally = True
try:
    do_something()
except BaseException as e:
    # prevent finally block from preempting BaseExceptions
    run_finally = isinstance(e, Exception)
    raise
finally:
    if run_finally:
        cleanup()

[hoodmane]

I think the logic all makes sense but is a bit messy and confusing. I also have some lingering doubts about whether we really need this, do we have any examples of wsgi apps that break without it?

[ryanking13]

do we have any examples of wsgi apps that break without it?

Not yet. Maybe we can wait until we found a compelling example that needs this. For now people can manually use asyncio.Lock around the fetch call.