Add a RwLock around module subscriptions to prevent a race condition

[coolreader18]

Description of Changes

Fixes that issue. From a comment I added in module_subscription_actor:

Take a lock on our subscriptions [before we commit the tx]. Otherwise, we could have a race condition where we commit the tx, someone adds a subscription and receives this tx as an update, and then receives the update again when we broadcast_event.

Expected complexity level and risk

2

[coolreader18]

@joshua-spacetime re: point 2, this is based off #791, which is why there's unrelated changes

[coolreader18]

Also, point 1: ...how would that work? Adding a subscriber needs mutable access to the vec. Unless we write/find a concurrent mutable data structure, that write lock is physically necessary to modify the subscriber list.

[joshua-spacetime]

@coolreader18

how would that work? Adding a subscriber needs mutable access to the vec.

My point was that the rest of the time (aside from mutating the list) we don't need exclusive access. HOWEVER, I would actually like to rescind my comment and say that we should keep it the way you have it, because I do think it's a good idea to keep performance and correctness fixes separate.

re: point 2

It still seems like there are more changes that are not related to the new RwLock. I'd really like this patch to only contain those changes necessary to fix the race condition. Even the performance related changes, which I originally suggested we do here, should be made in a separate patch.

[Shubham8287]

Resolved my comments, as we can merge it for fixing #788, I am writting a separate patch to make add_subscription concurrent.

[coolreader18]

It still seems like there are more changes that are not related to the new RwLock

I don't think there are - switching from an actor to shared state required touching other files, but we can't have a lock around shared state if the only access is via a message channel. Could you point to what specifically you think should be separate?