websocket: prevent deadlocks

[HaraldNordgren]

Follow-up to #402 (comment).

[HaraldNordgren]

I repeatedly amended and pushed my commit to re-run the tests. The 8th time it failed, it seems there is a deadlock in the code.

[HaraldNordgren]

Hi @benjaminjkraft, I'm at a bit of a loss here.

The deadlock in the tests seems going be stemming from the forwardData will still holding a lock.

However, deleting it exposes a situation where we can end up writing on a closed channel. That gives a panic which could be caught and ignored. But that seems like a very bad way to handle problems.

There seems to be an inherent race condition between checking isClosing and actually using channel. It's seems tricky to actually make this atomic.

Do you have any ideas?

[benjaminjkraft]

Hmm, this gets kinda complicated. I would need to think some more to see if I think it's right (for example I sure don't recall the rules for atomics). It feels like it's just sweeping problems under the rug.

I was too lazy to think so I gave this to Claude, who refactored to have just one lock (in the client); this passes the tests at least and seems less likely to have a hidden deadlock, although I'm not sure if I fully understand what invariants we need to ensure. Does having two actually let us be meaningfully more granular? (Or, can we ensure we always take the two locks out in some order? And we should probably regardless ensure we never hold a lock during network operations or calls to user-controlled code.)

[HaraldNordgren]

@benjaminjkraft Makes sense, I added some comment on the PR. Thanks for the help! ❤️