Skip to content

core: Handle ProxySelector IllegalArgumentException - #13053

Open
kkmurthyt21 wants to merge 1 commit into
grpc:masterfrom
kkmurthyt21:core-13052-proxy-selector
Open

core: Handle ProxySelector IllegalArgumentException#13053
kkmurthyt21 wants to merge 1 commit into
grpc:masterfrom
kkmurthyt21:core-13052-proxy-selector

Conversation

@kkmurthyt21

Copy link
Copy Markdown

Fixes #13052.

Android’s DefaultProxySelector.select() may throw IllegalArgumentException when the system proxy configuration contains an invalid port. This unchecked exception currently escapes the DNS resolver task and can terminate the process.

This change catches the exception at the ProxySelector.select() call and wraps it in an IOException. The original cause is preserved, and the exception message identifies the ProxySelector implementation.

DnsNameResolver already handles an IOException from proxy detection as an UNAVAILABLE name-resolution result. This allows the resolver to fail cleanly instead of letting the unchecked exception escape.

This intentionally avoids silently falling back to a direct connection. It follows the fail-cleanly approach used for invalid ProxySelector results in #12793 and avoids bypassing a configured proxy.

Testing

  1. Added a regression test verifying that the exception is wrapped, the original cause is preserved, and the ProxySelector implementation is identified.
  2. Ran .\gradlew.bat :grpc-core:test --tests "io.grpc.internal.ProxyDetectorImplTest"
  3. All 11 tests passed on Windows with Temurin JDK 17.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Sep 13, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: kkmurthyt21 / name: Krishna Kandi (bc01614)

@AgraVator

Copy link
Copy Markdown
Contributor

Thanks for picking this up. Approach is right, a few things need changing.

The test doesn't cover the actual bug. ProxyDetectorImpl throwing IOException was already
handled before this PR the crash is the unchecked exception escaping DnsNameResolver$Resolve.run()
onto the executor thread. As written the test passes on a build that still crashes. There's a
pattern to copy in DnsNameResolverTest around L519 (thenThrow(new IOException())); the same with
an IllegalArgumentException, asserting the listener gets a result, fails on master today.

Related: select() isn't the only unguarded call on that path. A few lines down
requestPasswordAuthentication() lands in java.net.Authenticator, which apps install via
setDefault() same thread, same crash. So this also wants a catch around detectProxy() in
Resolve.run() converting unchecked to IOException, and I'd widen the catch here to
RuntimeException rather than just IAE.

Also worth knowing: the message you're building never reaches the user, because DnsNameResolver
overrides the description with "Unable to resolve host " + host. I've argued for falling back to
no-proxy on the issue instead probably worth waiting for that to settle before reworking, no point
doing it twice.

IOException exception =
assertThrows(IOException.class, () -> proxyDetector.proxyFor(destination));

assertEquals(cause, exception.getCause());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: assertSame for the cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Android: ProxyDetectorImpl crashes when DefaultProxySelector contains an invalid proxy port

2 participants