google / cronet-transport-for-okhttp

This package allows OkHttp and Retrofit users to use Cronet as their transport layer, benefiting from features like QUIC/HTTP3 support or connection migration.
Apache License 2.0
452 stars 34 forks source link

Cronet with Retrofit is crashing due to file-descriptor leaks #15

Open marius-bardan opened 1 year ago

marius-bardan commented 1 year ago
CronetCallFactory.newBuilder(engine).build()
val builder = Retrofit.Builder()
builder.callFactory(factory)
// use builder with OkHttp

Before enabling Cronet with Retrofit using above snippet, make sure to chek the file-descriptor count, via:

  1. [standard emulator] adb shell
  2. su
  3. ls -l /proc/$(pidof APP_PACKAGE_NAME)/fd | wc [we're interested in the first value]

Running the same commands again, after we enable Cronet with Retrofit almost doubled the number of opened file descriptors for my app, which on some devices is causing a crash, since it hits the limit per one app.

edechamps-Google commented 1 year ago

Thanks for the feedback.

Using the sample app, and the following command on an ADB root shell:

for fd in /proc/$(pidof com.google.samples.cronet.okhttptransport)/fd/*; do readlink "$fd"; done | sort

FD counts:

4 additional FDs when sending requests using pure OkHttp:

+/dev/ashmem
+pipe:[93397]
+pipe:[93397]
+socket:[93392]

20 additional FDs when initializing Cronet:

+/data/app/com.google.android.gms-44whme714CrlclOYAuJT3w==/base.apk
+/data/user_de/0/com.google.android.gms/app_chimera/m/00000001/CronetDynamite.apk
+/data/user_de/0/com.google.android.gms/app_chimera/m/00000001/CronetDynamite.apk
+/dev/ashmem
+/dev/urandom
+anon_inode:[eventfd]
+anon_inode:[eventfd]
+anon_inode:[eventpoll]
+anon_inode:[eventpoll]
+anon_inode:[eventpoll]
+anon_inode:[timerfd]
+anon_inode:[timerfd]
+pipe:[95093]
+pipe:[95093]
+pipe:[95353]
+pipe:[95353]
+socket:[95091]
+socket:[95092]
+socket:[95351]
+socket:[95352]

2 additional FDs when sending requests through Cronet:

+socket:[96661]
+socket:[98600]

So using Cronet results in 18 additional FDs being used, compared to pure OkHttp.

I also looked at the number of FDs being opened per contacted destination domain, but I couldn't find a clear regression between pure OkHttp and Cronet-backed OkHttp in that regard - they both seem to use 1 to 2 FD per destination domain.

To clarify, is it the 20 FDs fixed cost from Cronet initialization that's bothering you or are you observing behavior that isn't covered in the above?

marius-bardan commented 1 year ago

I think it depends on the number of requests per app. In case of an enterpise app with tens of requests being executed, we're hitting the process limit of opened FDs and OS kills the app. We're not doing anything extra here - just using OkHttp vanilla vs using it with the Cronet interceptor (as basic as it can get).

Danstahrg commented 1 year ago

I've tried issuing 1k concurrent OkHttp requests to the top 1k websites as indicated by this list: https://gist.github.com/bejaneps/ba8d8eed85b0c289a05c750b3d825f61. Like Etienne, I haven't experienced any issues with the setup and the number of open descriptors looks roughly consistent as the requests re being processed, with the exception of Cronet being a bit more aggressive with keeping them open towards the end. This was using the default Pixel 6 emulator with Android 13.

If you can reliably reproduce the issue in a sandbox setup repro steps would be much appreciated.

marius-bardan commented 1 year ago

@Danstahrg can you please share the sample app code for that 1k requests test?