search

twilio / twilio-java

A Java library for communicating with the Twilio REST API and generating TwiML.
MIT License
484 stars 425 forks source link

Potential infinite timeout caused by not setting HttpClientBuilder.setDefaultSocketConfig? #663

Closed dpoineau closed 2 years ago

dpoineau commented 2 years ago

Issue Summary

Currently in NetworkHttpClient, HttpClientBuilder.setDefaultRequestConfig is used to set socket and connect timeouts for http requests performed by the library. However, per this issue, those settings are not applied to initial SSL handshake or CONNECT requests, which could lead to requests hanging indefinitely: https://issues.apache.org/jira/browse/HTTPCLIENT-1892

From the comments on the issue:

You are using HTTP request configuration, are you not? I do not know what your expectation is but request level configuration applies only once the connection route has been fully established. It does not apply to SSL handshakes or CONNECT requests. You need to configure socket properties applied by the connection manager upon connection creation.

For more context: with the issues that Twilio had on Thursday, Dec 10, we noticed that we were not receiving errors, but that message send requests were hanging indefinitely and never returning. I believe that this was potentially caused by not using setDefaultSocketConfig, which does seem to have an infinite timeout set by default: https://www.javadoc.io/doc/org.apache.httpcomponents/httpcore/latest/org/apache/http/config/SocketConfig.html

Since a pooling connection manager is used, this infinite timeout from setDefaultSocketConfig could have been hit if the manager was needing to create new connections to handle new requests (as opposed to using existing connections that would have had the setDefaultRequestConfig timeouts applied). I can't guarantee that this is what was happening, but I suspect that it may have been in our case.

Requested change

Can the library set a default timeout for this value, by using HttpClientBuilder.setDefaultSocketConfig? I think setting this to the same SOCKET_TIMEOUT that is used for setDefaultRequestConfig would make sense.

Current library timeout setting code

In https://github.com/twilio/twilio-java/blob/main/src/main/java/com/twilio/http/NetworkHttpClient.java#L67

        client = clientBuilder
            .setConnectionManager(connectionManager)
            .setDefaultRequestConfig(config)
            .setDefaultHeaders(headers)
            .setRedirectStrategy(this.getRedirectStrategy())
            .build();

Technical details: