Okhttp: `okhttp3.internal.Util.closeQuietly` isn't that quite

[bsideup]

It seems that an issue with the underlying buffer / connection may lead to an Exception (or even Error!) thrown from close method of the response. An example:

    java.lang.AssertionError: java.io.EOFException
        at okio.Buffer.clear(Buffer.java:932)
        at okio.RealBufferedSource.close(RealBufferedSource.java:477)
        at okhttp3.internal.Util.closeQuietly(Util.java:139)

What surprised us is Util.closeQuietly in the stacktrace.
Isn't it supposed to catch such errors and e.g. log instead of re-throwing?

[yschimke]

Yes, probably two questions to answer

1) Should closeQuietly throw RuntimeException or AssertionErrors?
2) Should okio ever turn a user input or end of stream into a non IOException?

https://twitter.com/bsideup/status/1273169998018752512

[swankjesse]

This particular crash is caused by unsafe concurrent access to a Buffer. Perhaps one code path is calling close() while another code path is attempting to read? Okio streams are not safe for concurrent use, and calling close() while another thread is reading or writing is unsupported.

The word _quietly_ in this context is intended to hide IOExceptions only. The AssertionError here is an invariant being violated, and I don't think it makes sense to attempt to recover from unexpected crashes.

[bsideup]

Perhaps one code path is calling close() while another code path is attempting to read? Okio streams are not safe for concurrent use, and calling close() while another thread is reading or writing is unsupported.

Oh wow, TIL! It may be problematic because we return InputStream to the user but close on our end on certain conditions. Could you please advice how to prevent errors here?

The word quietly in this context is intended to hide IOExceptions only. The AssertionError here is an invariant being violated, and I don't think it makes sense to attempt to recover from unexpected crashes.

FTR we've seen other exceptions (like AIOOBE from the tweet), and the expectation was that closeQuietly would suppress _any_ error (except the ones caused by JVM's misbehaviour like OOM or similar)

[JakeWharton]

If it had that behavior it would hide bugs, so perhaps we should change its name to more clearly indicate that we're ignoring only IOException and its subtypes.

[swankjesse]

I think you want cancel(), not close:

• Call.cancel() halts forward progress on a call. It does not release any resources.
• Source.close() and all things that ultimately call that release resources. Closing any of these is sufficient: InputStream, Response, ResponseBody.

Too bad about confusion with closeQuietly(). It's not public API so I assume if you’re reading the code you’re also seeing the implementation.

[yschimke]

Should we flag this type of problem in debug modes? E.g. check for concurrent calls?

[swankjesse]

We could do some basic stuff, like what ArrayList does with its modCount. But there‘s a potential performance cost, especially if we used volatile to tell colliding threads about each other.

[yschimke]

OK, so multiple works arounds or fixes

• don't access a Buffer in multiple threads concurrently
• use cancel

As far as fixes go

• This would be in Okio project, maybe more explanation ("concurrent access?") in Buffer assertion check

We want this error otherwise we hide real problems, hence the method is doing the right thing.

No fix possible within the scope of OkHttp, so closing this out. Thanks for the report @bsideup