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

Rephrase responseTimeout Javadoc #1620

Merged
merged 5 commits into from May 6, 2021
Merged

Rephrase responseTimeout Javadoc #1620

merged 5 commits into from May 6, 2021

Conversation

hisener
Copy link
Contributor

@hisener hisener commented May 5, 2021

The #responseTimeout Javadoc and Netty's ReadTimeoutHandler Javadoc don't seem to match.

Raises a ReadTimeoutException when no data was read within a certain period of time.

This PR tries to clarify the response timeout configuration.

Also, does it make sense to introduce #readTimeout and deprecate these methods as their name is misleading?

Copy link

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Agree that #readTimeout would be a clearer name, as #responseTimeout hints at "time to first byte" semantics.

@violetagg
Copy link
Member

violetagg commented May 5, 2021

Agree that #readTimeout would be a clearer name, as #responseTimeout hints at "time to first byte" semantics.

Can you clarify what you mean with readTimeout?
The response timeout setting is applied once the request is sent and removed once the response is received. It doesn't apply to the whole request/response.
The setting doesn't stay in place when the connection is returned to the pool.

If you need idle timeout you need to consider connection pool's maxIdleTime.

@violetagg
Copy link
Member

@hisener
Copy link
Contributor Author

hisener commented May 6, 2021

Also, does it make sense to introduce #readTimeout and deprecate these methods as their name is misleading?

^ Similar to this question, after these changes, does it make sense to deprecate the responseTimeout method and introduce maxReadOperationInterval instead (at some point)? :)

@violetagg
Copy link
Member

Also, does it make sense to introduce #readTimeout and deprecate these methods as their name is misleading?

^ Similar to this question, after these changes, does it make sense to deprecate the responseTimeout method and introduce maxReadOperationInterval instead (at some point)? :)

Please open a new PR and let's discuss it there.

@hisener
Copy link
Contributor Author

hisener commented May 6, 2021

Also, does it make sense to introduce #readTimeout and deprecate these methods as their name is misleading?

^ Similar to this question, after these changes, does it make sense to deprecate the responseTimeout method and introduce maxReadOperationInterval instead (at some point)? :)

Please open a new PR and let's discuss it there.

Sure. Then, I take this as yes to my question. :)

@violetagg
Copy link
Member

@reactor/netty-team PTAL

@violetagg violetagg requested a review from a team May 6, 2021 11:31
@violetagg violetagg added this to the 1.0.7 milestone May 6, 2021
@violetagg
Copy link
Member

@hisener @Stephan202 Thanks for the discussion and the PR!

@violetagg
Copy link
Member

@simonbasle Thanks for the review

@violetagg violetagg merged commit 263ba19 into reactor:main May 6, 2021
@Stephan202 Stephan202 deleted the hsener/response-timeout-doc branch May 6, 2021 15:07
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.

None yet

5 participants