Potential incompatiblity with apollographql/apollo-kotlin.

[duzinkie]

I'm researching moving our GQL clients (that use https://github.com/apollographql/apollo-kotlin) to use cronet HTTP engine (to leverage http3/quic) and since we rely heavily on OkHttp, decided to use the CronetInterceptor from this package to minimize the changes to the application I'm working on.

(apologies if the description below sounds too vague/simplistic, I do not know the domain very well and might not have the right vocabulary)

While working on this transition, I have noticed, that, when using GQL batching, along with the interceptor from this package our app would very often (though not always) "hang" when sending requests.

I was able to check that:

• https://github.com/apollographql/apollo-kotlin/blob/main/libraries/apollo-runtime/src/commonMain/kotlin/com/apollographql/apollo3/network/http/BatchingHttpInterceptor.kt#L141 sets the request contentLength to -1
• this leads to cronet interceptor using StreamingRequestBodyConverter. Eventually, control is being handed over to the cronet engine itself, and afaik it (oddly) attempts to read the GQL request twice after which.. the hang occurs.

I am able to circumvent the issue by providing an okhttp interceptor that reads the entire request and sets the content length to the length of what it just read before handing over to cronet (it's implementation is simple so I'll skip it - it's similar to the InMemoryRequestBodyConverter really)

This behavior occurs far less frequently when the app is being debugged and breakpoints are present at various stages, suggesting it might be some sort of deadlock/race condition, but I'm unable to pinpoint that exactly. I did notice the usage of .allowDirectExecutor(); in the interceptor code which is documented as potentially dangerous but can't say if this is significant.

Even though I have a workaround (our requests are small so I think we'll be fine flushing request to force the use of InMemoryRequestBodyConverter but I figured you might be interested to know that there's a potential incompatibility here. I've also reported the issue in apollo as well under apollographql/apollo-kotlin#5847. Also tothing in "Incompatibilities" section of the readme stood out as potential root cause, but again I'm lacking expertise to really tell.

Of course I understand it can also be the case that it's neither apollo nor cronet-transport but our own integration with those that causes the problem, or that this doesn't align with your own roadmap so feel free to resolve this as needed.

Let me know if you'd like me to share any more information, and thank you for any assistance you can provide.

[aymanm419]

I think this might be related to one of the bugs that was found lately in Cronet UploadDataStream. An undefined behavior will occur if onReadSucceeded is called when we didn't read any data into the bytebuffer. It has been fixed in the latest versions of Cronet where such cases will lead to the failure of the request itself, we believe that reading an empty buffer is an illegal case.

I am not entirely sure if this is your issue but you might want to double-check that you never do an empty chunked body except for the last chunk to indicate the end of upload body.

See https://chromium-review.googlesource.com/c/chromium/src/+/5426015 for more information

[duzinkie]

It has been fixed in the latest versions of Cronet where such cases will lead to the failure of the request itself, we believe that reading an empty buffer is an illegal case.

@aymanm419 I'm on https://mvnrepository.com/artifact/org.chromium.net/cronet-embedded/119.6045.31 - which version should I use instead?

[aymanm419]

The implementation that has the new fix should be in Cronet v126.0.6472.0 which we did not release yet. We are a little slow with releasing embedded versions, if I may ask, have you considered switching to GMS-based Cronet? You won't then have to bundle Cronet statically freeing up around ~3MB of your APK's / Library size.

https://mvnrepository.com/artifact/com.google.android.gms/play-services-cronet

The good thing about the above is that users will always get the newer versions of Cronet implementation regularly, the downside is that you don't control that.

[duzinkie]

if I may ask, have you considered switching to GMS-based Cronet?

Yeah, we did. Loading/latencies are more important to us than larger APK downloads, so we prefer the embedded version (docs and our old research suggest gms cronet would be slower to load, though I have not verified that myself)

That being said, I could check if the error reproduces when switching to GMS version of cronet, thanks for the suggestion.