Skip to content

Merge connect and handshake timeouts in AbstractIOSessionPool#102

Merged
rschmitt merged 1 commit intoapache:masterfrom
rschmitt:master
Jan 25, 2019
Merged

Merge connect and handshake timeouts in AbstractIOSessionPool#102
rschmitt merged 1 commit intoapache:masterfrom
rschmitt:master

Conversation

@rschmitt
Copy link
Contributor

This change essentially reverts a portion of 40cae22 that appears to
have been unnecessary. Since the requestTimeout parameter is always a
connection timeout (at least in the client-side code), we don't need to
dedicate an additional parameter to the TLS handshake timeout value: we
simply use the requestTimeout (which has been renamed connectTimeout
for clarity).

This change essentially reverts a portion of 40cae22 that appears to
have been unnecessary. Since the `requestTimeout` parameter is always a
connection timeout (at least in the client-side code), we don't need to
dedicate an additional parameter to the TLS handshake timeout value: we
simply use the `requestTimeout` (which has been renamed `connectTimeout`
for clarity).
@rschmitt
Copy link
Contributor Author

I'm not very confident about the rename from requestTimeout -> connectTimeout, because it appears that this parameter sometimes behaves as a connectTimeout and other times behaves as a connectionRequestTimeout. Suggestions and clarifications welcome!

@ok2c
Copy link
Member

ok2c commented Jan 24, 2019

@rschmitt connectTimeout sounds correct to me. I could not find any evidence of this variable being used as connectionRequestTimeout by AbstractIOSessionPool. Have I missed it somehow?

@rschmitt
Copy link
Contributor Author

I guess you're right. I suppose that makes sense, since this class is pooling sessions and not connections. (It's just a quirk of terminology that we also "connect" to establish a session.)

@rschmitt rschmitt merged commit 8f3b58d into apache:master Jan 25, 2019
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.

2 participants