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

readTimeout and connectTimeout have no affect if server does return HTTP headers #49

Closed
rymsha opened this issue Mar 24, 2022 · 6 comments · Fixed by #51
Closed

readTimeout and connectTimeout have no affect if server does return HTTP headers #49

rymsha opened this issue Mar 24, 2022 · 6 comments · Fixed by #51

Comments

@rymsha
Copy link

rymsha commented Mar 24, 2022

When server establishes a socket connection, but never sends anything back HttpCleint gets stuck

	at java.base@11.0.14/jdk.internal.misc.Unsafe.park(Native Method)
	at java.base@11.0.14/java.util.concurrent.locks.LockSupport.park(LockSupport.java:194)
	at java.base@11.0.14/java.util.concurrent.CompletableFuture$Signaller.block(CompletableFuture.java:1796)
	at java.base@11.0.14/java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3128)
	at java.base@11.0.14/java.util.concurrent.CompletableFuture.waitingGet(CompletableFuture.java:1823)
	at java.base@11.0.14/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1998)
	at platform/java.net.http@11.0.14/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:541)
	at platform/java.net.http@11.0.14/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
	at app//com.github.mizosoft.methanol.Methanol$InterceptorChain.forward(Methanol.java:775)
	at app//com.github.mizosoft.methanol.Methanol$ReadTimeoutInterceptor.intercept(Methanol.java:944)
	at app//com.github.mizosoft.methanol.Methanol$InterceptorChain.forward(Methanol.java:780)
	at app//com.github.mizosoft.methanol.Methanol$AutoDecompressingInterceptor.intercept(Methanol.java:884)
	at app//com.github.mizosoft.methanol.Methanol$InterceptorChain.forward(Methanol.java:780)
	at app//com.github.mizosoft.methanol.Methanol$RequestRewritingInterceptor.intercept(Methanol.java:828)
	at app//com.github.mizosoft.methanol.Methanol$InterceptorChain.forward(Methanol.java:780)
	at app//com.github.mizosoft.methanol.Methanol.send(Methanol.java:300)

I don't think it is a Methanol issue itself, but maybe at can be fixed with a little help from Methanol ?

@mizosoft
Copy link
Owner

Hi @rymsha, can you provide a test case? readTimeout should complete the request exceptionally if a read couldn't be fulfilled within the timeout (e.g. a read() from an InputStream, etc.). That's only possible after the headers are received. Otherwise, readTimeout won't have the chance to schedule any timeouts.

If the server establishes a connection but never finishes sending the headers, neither readTimeout nor connectTimeout can do much. If that's your case, have you tried setting a request timeout?

@rymsha
Copy link
Author

rymsha commented Mar 25, 2022

Here is an example. okhttp3 mock server is used simulate such conditions

import java.net.URI;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.time.Duration;

import com.github.mizosoft.methanol.Methanol;

import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.MockWebServer;
import okhttp3.mockwebserver.SocketPolicy;

public class HttpClientNoResponse
{
    public static void main( String[] args )
        throws Exception
    {
        MockWebServer server = new MockWebServer();
        try
        {
            MockResponse response = new MockResponse();
            response = response.setSocketPolicy( SocketPolicy.NO_RESPONSE );
            // OR
            // response = response.setSocketPolicy( SocketPolicy.STALL_SOCKET_AT_START );
            
            server.enqueue( response );

            server.start();

            final var client =
                Methanol.newBuilder().readTimeout( Duration.ofMillis( 10000 ) ).connectTimeout( Duration.ofMillis( 5000 ) ).build();
            final URI uri = URI.create( "http://" + server.getHostName() + ":" + server.getPort() );
            client.send( HttpRequest.newBuilder().uri( uri ).build(), HttpResponse.BodyHandlers.discarding() );

        }
        finally
        {
            server.shutdown();
        }
    }
}

requestTimeout it is too aggressive: it will drop off the request if response is not received fully before timeout. For responses that take long time this is not an option.

@rymsha
Copy link
Author

rymsha commented Mar 25, 2022

Maybe it is possible to implement a similar functionality as for requestTimeout. With the only that TimeoutException won't trigger if response is partially received (even a single byte, or better until readTimeout can step in).

@mizosoft
Copy link
Owner

I see. I got what you mean about requestTimeout. I believe you're looking for something along the lines of headersTimeout: raise an exception if headers aren't received without the timeout, then readTimeout (if set) can step in for single reads. Sounds reasonable, I will look into it. Thanks for reporting.

@rymsha
Copy link
Author

rymsha commented Apr 27, 2022

Great that you found time to make this!
Would it be released any time soon?

@mizosoft
Copy link
Owner

Should be able to finally cut the 1.7.0 release in about a week.

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 a pull request may close this issue.

2 participants