Thread-safety in Async Worker

[Bi0max]

Hello, @miguelgrinberg.
Firstly, thank you for this library! I have already used it in a Flask project, worked really nice for me.
But now I have a question concerning another FastAPI project.

I have seen in the documentation that emit method is not thread-safe. But it does not say more about how to tackle the issue exactly and in which cases.

I have read several issues about it:

1. RuntimeError: Second simultaneous read on fileno #13
2. Enable socketio.emit() to work in multiple threads Flask-SocketIO#268
3. https://stackoverflow.com/questions/38777793/is-flask-socketios-emit-function-thread-safe
4. Client doesn't receive emits from Thread #99
5. Making library thread-safety #596

But I am still not sure, whether I should use asynchronous Lock and where?

So in short:

1. I have a FastAPI webapp with socketio.AsyncServer, which is used to connect and disconnect the client. For now, no events are emitted directly from FastAPI route handlers.
2. Some events are emitted from fastapi.BackgroundTasks.
3. I have a few separate async workers (which live in different processes and listen to AWS SQS), which also emit events to clients.
4. In both cases socketio.AsyncRedisManager(write_only=True) seems to be enough for my needs.

The question is:

1. Do I need to add any Locks in fastapi.BackgroundTasks to emit events?
2. Do I need to add any Locks in my async workers to emit events?

Thank you!

[miguelgrinberg]

In general I'd say the answer is yes for 1 and no for 2.

But really you need to look at this in detail to determine exactly when you need to lock. Given that you are using a message queue, the first consideration is wether the writes to the message queue are safe to issue from multiple tasks. If they are, then the emits from your background jobs should be safe as well.

If your server will never have two background jobs emitting at the same time, then this is also a good indicator that you can get away without using locks.

The external workers should be fine as long as they don't emit from multiple tasks.

[Bi0max]

Given that you are using a message queue, the first consideration is wether the writes to the message queue are safe to issue from multiple tasks.

You mean Redis queue here?
How do we determine that the writes to it are safe? If we do not emit simultaneously to the same client?

If your server will never have two background jobs emitting at the same time, then this is also a good indicator that you can get away without using locks.

This could happen actually.

The external workers should be fine as long as they don't emit from multiple tasks.

Here also, external workers each may run a certain amount of async tasks simultaneously.
Some of these tasks may emit events.

And in general, on which level does it become not thread-safe?
Do I have to lock just for a certain client or for the whole instance of AsyncServer?

[miguelgrinberg]

How do we determine that the writes to it are safe? If we do not emit simultaneously to the same client?

This is a property of the Redis library. I honestly do not know if aioredis can be used concurrently or not, but I'd like to think that this is a supported workflow, so more than likely you should be okay without locking.

And in general, on which level does it become not thread-safe?
Do I have to lock just for a certain client or for the whole instance of AsyncServer?

I'm not sure I understand this. If the aioredis works fine from multiple tasks (this isn't threads, it is tasks), then there is no need to lock. If aioredis can get confused if it is used concurrently, then you should add a critical section around each emit to ensure that there are never two emits running concurrently. This is for the whole processes.

But again, it is likely this is fine as it is. :)

[Bi0max]

Ah, I actually assumed from the beginning that aioredis supports concurrency, otherwise it would be not so useful. Hopefully, this is the case, lol :)

So, the whole problem comes only from the underlying message queue? If we assume, that aioredis handles concurrent write correctly, then nothing has to be done?

Because I was confused by the following note in the docs of AsyncServer.emit:

Note: this method is not designed to be used concurrently. If multiple tasks are emitting at the same time to the same client connection, then messages composed of multiple packets may end up being sent in an incorrect sequence. Use standard concurrency solutions (such as a Lock object) to prevent this situation.

This is what I meant by whether it is concurrency-unsafe only on the same client level:

If multiple tasks are emitting at the same time to the same client connection

[miguelgrinberg]

When you use a message queue, all events are written to the queue first, then taken from there by another task that performs the emit. The note that you extracted from the docs is written generically, and covering the non-message queue case as well, which is more complex.

Basically you have to decide if you want to look at your particular situation to decide if you need to lock or not, or if you prefer to use the safe general advice. If the former, then as I said, I think you are probably fine w/o locking. But if you want to be safe and don't want to do a detailed analysis of your message queue and how it behaves under concurrency, then you should use a lock.

[Bi0max]

Thank you for answers, Miguel!
Really appreciate, that you took time to answer so quickly.

I will then go firstly w/o locking, and if at some point there will be more traffic/unexpected problems, I'll make a deeper analysis.