Multiple subscribes before client connects leads to unstable websocket clients #37
Comments
|
Good catch, it's totally a big issue. We need to fix that ASAP, it will cause trouble on most systems. |
|
I think the bigger issue is the linked change where I made disconnect() async. But there are also some other assumptions made and corners cut. I think the issues will actually be resolved when we simplify the roles of each method. |
|
In fact, now that we're using OkHttp for websockets, I think we can revert that |
|
Did you find that you needed to put disconnect on a separate thread after OkHttp? |
|
No, that change was before the OkHttp change merged. And I'm not even sure how to reproduce the issue, just saw it in one of our betas via crashlytics. But the problem is that OkHttp likely would not have that issue, because ALL network traffic is handled on a separate thread for the OkHttpClient. I'll try some more tests with the async-disconnect commit reverted, and see if that fixes the weirdness I was seeing. |
|
I've confirmed that with OkHttpClient, we no longer need the |
This reverts commit 3b0041d The theory is that using OkHttp now, all networking is handled on the correct thread automatically. Closes parse-community#37
* Revert async-disconnect This reverts commit 3b0041d The theory is that using OkHttp now, all networking is handled on the correct thread automatically. Closes #37 * Clarify the logic of initiating a connection during subscribe() This also creates a new connectIfNeeded() method, which can be used to initiate a connection if not already connected or connecting. The original intent was to connectIfNeeded() during subscribe. While some may argue that's not necessary, and the client should instead be explicit about what is intended, this change preserves that original intention. As it stands, the only way to queue up pending subscriptions _without_ initiating a connection would be to call disconnect() first, even though the client hasn't been connected yet. That would set the user-initiated flag, which would therefore refuse to auto-connect during subscribe(). * Remove PausableExecutor Also introduces the ImmediateExecutor (same as what we used to have, before switching to PausableExecutor). Similar to, but simpler than, `bolts.BoltsExecutors#immediate()`.
Starting in 3b0041d, the following sequence of events leads to the websocket sending duplicate subscribe events:
client = getClient(..)client.subscribe(query1)client.subscribe(query2)After (1), the client is not yet connected. During (2), the subscription becomes "pending", and a call to
reconnect()is started. As of that point in time, the socket is still not connected. During (3), the subscription is still added to the pending subscriptions list, but because thereconnect()from (2) is actually asynchronous, it probably hasn't started yet (or may be partially started), so it also tries to callreconnect()again.That leads to the first reconnect call (2) becoming useless because the (3) call disconnects and reconnects it as well.
However, the race condition leads to more trouble, as the connection attempt of (2) still succeeds, and therefore we still attempt to send the pending subscriptions. Then the connection attempt of (3) also succeeds, and the pending subscriptions are sent again.
We should be able to use the state of the websocket client to determine whether we can start connecting immediately, or we need to initiate the process of disconnecting (or in some cases, we may not need to disconnect/reconnect at all -- i.e., if we're in the
CONNECTINGstate, we should probably just wait for that to finish connecting (or fail))The text was updated successfully, but these errors were encountered: