Connect Timeout - Thread.Interrupt() doesn't cancel call.execute() when we're stuck at the connection state

[om3g4zell]

Describe the bug

When the server is unreachable and isn't able to answer to the connect request. Using
Thread.interrupt() doesn't cancel the call and only unblock after the connectTimeout
property has passed.

Expected behaviour

I Exptect Thread.interrupt() to have the same behaviour as call.cancel() -> Cancel the call even when stuck at the connecting state.

Client information

OS : Unix
OKHttp Version : 4.9.0
JDK : AdoptOpenJDK 11

Step to reproduce

Make a call that would result in a connectTimeout and try to interrupt it with Thread.interrupt()

Here is a java test (using assertJ)

    @Test
    void interupt_on_connect( ) throws InterruptedException {

        var connectTimeout = Duration.ofMillis( 500 );
        var client = new OkHttpClient.Builder( )
                .connectTimeout( connectTimeout )
                .build( );

        var call = client.newCall( new Request.Builder( )
                .url( new HttpUrl.Builder( )
                        .scheme( "http" )
                        // Host that don't listen packets nor reject them
                        .host( "10.255.255.1" )
                        .build( ) )
                .get( )
                .build( ) );

        var thread = new Thread( ( ) -> {
            try (var resp = call.execute( )) {
                fail( "Shouldn't succeed" );
            } catch (IOException e) {
                // NO-OP
            }
        } );
        thread.start( );
        // Test will fail using Thread.interrupt
        thread.interrupt( );

        // Test will succeed using call.cancel()
        //call.cancel();
        thread.join( connectTimeout.toMillis( ) );
        assertThat(thread.isAlive( ) ).isFalse();
    }

The test is passing using call.cancel() and failing using Thread.interrupt()

Here is a unit test that can be placed in CancelTest.kt

  @ParameterizedTest
  @ArgumentsSource(CancelModelParamProvider::class)
  fun cancelConnecting(mode: Pair<CancelMode, ConnectionType>) {
    val connectTimeout = Duration.ofMillis(500);

    setUp(mode)
    val client = client.newBuilder()
      .connectTimeout(connectTimeout)
      .build()
    val call = client.newCall(
      Request.Builder()
        .url(HttpUrl.Builder()
          .scheme(connectionType.name)
          // // Host that don't listen packets nor reject them
          .host("10.255.255.1")
          .build())
        .get()
        .build()
    )
    cancelLater(call, 100)
    try {
      call.execute()
      fail("")
    } catch (expected: IOException) {
      expected.printStackTrace()
      assertEquals(cancelMode == INTERRUPT, Thread.interrupted())

      // If connect timeout exception happens it means that the call hasn't been canceled
      assertThat(expected).isNotInstanceOf(SocketTimeoutException::class.java)
    }
  }

Test using the H2 ConnectionType will fail as it's not a valid shcheme.

Tests using call.cancel() will succeed.
Tests using Thread.interrupt() will fail after the connectTimeout delay.
The Thrown exception is

java.net.SocketTimeoutException: connect timed out

instead of

java.net.SocketException: Socket closed

with the call.cancel() method

[yschimke]

I don't think we are against improving our handling here, I think it's complicated because interrupts in frameworks/containers are awkward to get right for everyone, and it's unpredictable on different devices (Android vs JDK) and versions. Or with different socket implementations like Conscrypt.

See discussion in #1902 and a basic test covering interrupts here https://github.com/square/okhttp/pull/5937/files

If you can add a PR and clean maintainable fix it might be viable, but at the moment it's been easier to say that we expect clients to use the public API over assuming that interrupts are exactly equivalent to a cancel call.

[yschimke]

Closing, and to continue in #7185