Fix race condition in WebBleConnection #14
Conversation
WebBleConnection's constructor calls init(), which is async. Constructors can't be async, so this means that callers who instantiate WebBleConnection can end up with it in an invalid state after construction if init() hasn't completed by the time the caller tries to access class members that depend on init. I think the cleaner solution is to push the init() call to open(), which is async, so it can just await the results of init(), and the caller can trust that after calling open(), the connection is fully initialized. This is a second attempt (see 78a0037). The fix here is to defer emitting the connected event a by wrapping it in a setTimeout(..., 0), which prevents open() from firing the event before the client has a chance to set up an event handler for the 'connected' event. This matches the strategy we use in WebSerialConnection.
|
You can test this implementation in my Github Pages deployment that has this fix: https://mtlynch.github.io/meshcore.js/ |
|
This would work, but maybe it would just be clearer for the callers if they had to explicitly |
|
@kallanreed - Yeah, I agree that it still feels a bit brittle. I was trying to avoid pushing work onto existing callers, but if we're fine with a breaking change, I think it's a good idea to let the caller explicitly initiate the connection. |
|
I played around with this some more and I think pulling "init" out like I suggested isn't a good idea - to your original point of the comment thing should be initialized completely before it's returned. Maybe open should accept the onconnect callback instead? That would at least let onconnect code run. Just tossing out ideas. The proposed fix is probably fine. |
2 participants
WebBleConnection's constructor calls init(), which is async. Constructors can't be async, so this means that callers who instantiate WebBleConnection can end up with it in an invalid state after construction if init() hasn't completed by the time the caller tries to access class members that depend on init.
I think the cleaner solution is to push the init() call to open(), which is async, so it can just await the results of init(), and the caller can trust that after calling open(), the connection is fully initialized.
This is a second attempt (see 78a0037). The fix here is to defer emitting the connected event a by wrapping it in a setTimeout(..., 0), which prevents open() from firing the event before the client has a chance to set up an event handler for the 'connected' event. This matches the strategy we use in WebSerialConnection.