forresthopkinsa / StompProtocolAndroid

Websockets on Android
MIT License
11 stars 4 forks source link

Can't re-connect after error with OkHttp lib #6

Closed stairs closed 6 years ago

stairs commented 6 years ago

OkHttp websocket ERROR event means that socket is closed and no further calls to listener will be made. However, next piece of code only notifies the client that connection is closed when CLOSE event is received.

    void emitLifecycleEvent(@NonNull LifecycleEvent lifecycleEvent) {
        Log.d(TAG, "Emit lifecycle event: " + lifecycleEvent.getType().name());
        mLifecycleStream.onNext(lifecycleEvent);
        if (lifecycleEvent.getType().equals(LifecycleEvent.Type.CLOSED))
            mConnectionStream.onNext(false);
    }

As a result we get "Attempted to connect when already connected" when socket is actually closed.

stairs commented 6 years ago

I'll create a PR with fast fix but I won't be able to test right now. @forresthopkinsa Please, take a look and test if you can.

forresthopkinsa commented 6 years ago

Absolutely, I'll check it out when I get to work in about an hour.

forresthopkinsa commented 6 years ago

You're totally right. With JWS, messages can still be sent after an Error event, whereas with OkHttp, a Failure is a combination of an Error and a Close. The code should definitely reflect that.

stairs commented 6 years ago

Thanks a lot for fixing this one! Unfortunately, I've found another issue after this one was fixed. I'll report it separately.

souravbasu14 commented 6 years ago

Hi, I faced the reconnect problem after n/w disconnet/connect in the upstream project. i put my issue there [ref: https://github.com/NaikSoftware/StompProtocolAndroid/issues/87] where i saw the same discussion and came to this downstream project. I tried with 17.11.0 version but still facing the same reconnect problem. Is it the proper version to be checked or I am missing something.

forresthopkinsa commented 6 years ago

@souravbasu14 That fix wasn't included in the 17.11.0 release. It was released a day later, and it's scheduled to be included in 17.12.0. You can, however, use a non-release version, like so:

dependencies {
    compile 'com.github.forresthopkinsa:StompProtocolAndroid:965f1275ad'
}
souravbasu14 commented 6 years ago

@forresthopkinsa I did a quick verification with the above version. Below are my steps and observations:

  1. Once the network is disconnected, I am getting ERROR state. From there I called reconnect().
  2. On network connection, stompclient is getting connected and i receive OPEN state.
  3. Old subscribed topics were not restored. No data is coming after reconnect
  4. After step 2, OPEN and ERROR goes into a loop. After ~1min interval i get a ERROR state though my network doesn't change. And as per Step 1, i called reconnect(). So i get OPEN state. And this cycle continues. Also previous subscribed topics are not restored.

Also observed a crash which leads the app to crash. Attached is the crashlog which i received in crashlytics. crash.txt