square / okhttp

Square’s meticulous HTTP client for the JVM, Android, and GraalVM.
https://square.github.io/okhttp/
Apache License 2.0
45.71k stars 9.15k forks source link

Read not throwing exception when connection is closed (Server sent events) #2611

Closed marcelpinto closed 8 years ago

marcelpinto commented 8 years ago

I started an implementation of Server Sent Events using OkHttp as the network layer. The implementation is pretty straightforward.

The constructor takes the OkHttpClient and sets the timeout to 0 since we need to keep the connection alive.

    private EventCall(OkHttpClient client, Request request) {
        this.client = client.newBuilder()
                .readTimeout(0, TimeUnit.SECONDS)
                .writeTimeout(0, TimeUnit.SECONDS)
                .connectTimeout(0, TimeUnit.SECONDS)
                .retryOnConnectionFailure(true)
                .protocols(Collections.singletonList(Protocol.HTTP_1_1))
                .build();

        this.request = request.newBuilder().header("Accept", "text/event-stream").header("Cache-Control", "no-cache").build();
        this.call = this.client.newCall(request);
    }

Once the call is enqueued and successfully I use the StreamAllocation to created the buffer to read from. Then I block the thread with a read method that will only go out in the case of error.

private void createSSE(final EventListener listener) {
        StreamAllocation streamAllocation = Internal.instance.callEngineGetStreamAllocation(call);
        RealEvent source = StreamEvent.create(streamAllocation, listener);
        while (source.read()) {
        }
}

The read method is waiting for message from the server and parses them following the SSE specs

    public boolean read() {
        try {
            String line = source.readUtf8LineStrict();
            processLine(line);
        } catch (IOException e) {
            realEventListener.onError(e);
            return false;
        }
        return true;
    }

The code works, and the message is received correctly. The problem comes when I tried to handle network issues. For example, if the wifi connection is lost.

Two things happens in that case:

  1. The read operation throws an exception Read error: ssl=0xae45e000: I/O error during system call, connection timed out What is correct, since when have been disconnected
  2. The read operation keeps blocked and no error is thrown.

In the second case, if we connect again the WiFi and we send a message from the server, the read method will get unblocked and read it, but afterward, it won't read anything else.

The first case seems to happen more if we disconnect the wifi a bit before receiving an event, the second one is happening more often.

One solution would be to set a read timeout and try to reconnect. But this is against of what SSE defines.

I may have miss sth or I am doing something wrong. Let me know if you need further information.

marcelpinto commented 8 years ago

Seems that is device related...

Samsung Galaxy Note 4 Android 5.1.1 I tried with a sony xperia z5 and the exception is thrown same for a samsung s4.

swankjesse commented 8 years ago

Please do not use any classes in OkHttp’s internal package. Those are not public APIs and will change with each release without warning.

Once you’ve done that you should isolate this big into a test case and open a new issue. As is there’s no obvious thing to fix here.

marcelpinto commented 8 years ago

@swankjesse I used the Internal call since I wanted to follow how the okhttp-ws are created since the idea is to contribute it back to the project.

Anyways I switch it back to use the response.body().source() and same behavior.

I know it should come with a test case but I am not really sure how to do it. Since it requires a connection to a server that handles SSE and seems to be device specific. I will try to reproduce it with a mock server but I doubt I can. Any better idea?

Konsumierer commented 8 years ago

I have a similar problem.

Instead of using SSE, I use http-based long-polling. So, I also have long-running http requests:

try
{
    final Response httpResponse = httpClient.newCall( httpRequest ).execute();
    // ...
}
catch (final IOException e)
{
    LOG.error( "An error occurred.", e );
}

Now, when I disconnect the Wi-Fi, I expect the IOException to be instantly thrown and it is actually thrown on most devices. However, on the Samsung Galaxy S6 with Android 5.1.1 it is not thrown, instead a SocketTimeoutException is thrown after the timeout.

OkHttp version in use is 2.7.5.

@skimarxall, which version did you use? @swankjesse, could you please have a look at it? Although I am currently not able to provide you with a test case. Sorry ...

sschuberth commented 6 years ago

Looks like OkHttp 3.11.0 introduced experimental support for SSE after all. @skimarxall, does it make sense to merge https://github.com/heremaps/oksse with upstream then?

marcelpinto commented 6 years ago

I don't think we can merge it, but I will try to contribute back with lessons learn and improvements.