Skip to content

HTTPCLIENT-2386: Fix TLS handshake timeout handling #541

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arturobernalg
Copy link
Member

Fix TLS handshake timeout handling (HTTPCLIENT-2386)

Use socketTimeout for TLS handshake and add dedicated timeout exception.

- Use socketTimeout (not connectTimeout) as default TLS handshake timeout
- Throw TlsHandshakeTimeoutException for handshake timeouts
@arturobernalg arturobernalg requested a review from ok2c July 31, 2025 17:38

final SSLIOSession tlsSession = tlsSessionRef.get();
if (tlsSession != null && !tlsSession.isHandshakeComplete()) {
throw new TlsHandshakeTimeoutException("TLS handshake timed out after " + timeout);
Copy link
Member

Choose a reason for hiding this comment

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

@arturobernalg
I think we should add a timeout only constructor and have the message in that constructor so it's normalized and not duplicated in each call site.

final IOEventHandler handler = ensureHandler(currentSession);
handler.timeout(currentSession, timeout);
}


Copy link
Member

Choose a reason for hiding this comment

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

Extra line?

@ok2c
Copy link
Member

ok2c commented Jul 31, 2025

@arturobernalg I do not think we need this. The root cause of the problem is HTTPCLIENT-2386.

@arturobernalg
Copy link
Member Author

@arturobernalg I do not think we need this. The root cause of the problem is HTTPCLIENT-2386.

@ok2c This change is inspired by the discussion in mailing list thread: https://lists.apache.org/thread/11bzsrztjm9y2862wo3x5ysoo9s1f5f7.

@ok2c
Copy link
Member

ok2c commented Aug 1, 2025

@arturobernalg I do not think we need this. The root cause of the problem is HTTPCLIENT-2386.

@ok2c This change is inspired by the discussion in mailing list thread: https://lists.apache.org/thread/11bzsrztjm9y2862wo3x5ysoo9s1f5f7.

@arturobernalg I am aware. I wrote it. It would be an option if we had not found the cause of the issue.

@arturobernalg
Copy link
Member Author

@arturobernalg I do not think we need this. The root cause of the problem is HTTPCLIENT-2386.

@ok2c This change is inspired by the discussion in mailing list thread: https://lists.apache.org/thread/11bzsrztjm9y2862wo3x5ysoo9s1f5f7.

@arturobernalg I am aware. I wrote it. It would be an option if we had not found the cause of the issue.

@ok2c 100% agree. Trying to fix the issues on the client meanwhile.

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.

3 participants