Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for Conscrypt concurrency issue #6509

Closed
prbprbprb opened this issue Jan 14, 2021 · 4 comments · Fixed by #6533
Closed

Workaround for Conscrypt concurrency issue #6509

prbprbprb opened this issue Jan 14, 2021 · 4 comments · Fixed by #6533
Labels
enhancement Feature not a bug

Comments

@prbprbprb
Copy link

Android issue 177450597 looks to be due to a concurrency issue in Conscrypt where two threads race to close an SSLEngine-based SSLSocket and access to the underlying BIO is unsynchronized.

We already have a PR google/conscrypt#942 in-flight which addresses this, but realistically we're not going to get that onto the majority of Android 10 devices, so the bug is going to be around for some time, so maybe okhttp could consider quietly swallowing any NPE in closeQuietly()?

@prbprbprb prbprbprb added the enhancement Feature not a bug label Jan 14, 2021
@swankjesse
Copy link
Collaborator

Yeah, good idea. Want to submit a PR? Otherwise we’ll do.

Also I assume we should backport the fix to 3.12.x?

@prbprbprb
Copy link
Author

I'll have a look, it'll be good for me :)

Even if we mitigate it in closeQuietly(), the other thread in the race will still see exceptions some of the time, but I guess that's less of an issue because it's getting cancelled anyway (or returned to an executor, I don't know the architecture :)

@yschimke
Copy link
Collaborator

This also worried me... #6521

Is OkHttp failing in a bad state when the underlying SSL layer sends runtime errors?

@swankjesse
Copy link
Collaborator

Yeah good question. I think it's quite surprising that a runtime crash in the connection pool cleanup task could hang a call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature not a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants