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

Honor read timeout in reactive rest client #19223

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Aug 4, 2021

Before this change the read timeout only had an effect when doing
a synchronous call (and even then it didn't result in the proper Vert.x exception)

The connect timeout handling doesn't need any changes because it is properly handled
when org.jboss.resteasy.reactive.client.impl.ClientImpl is created

Fixes: #17526

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 4, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 8119002

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build ⚠️ Check → Logs Raw logs
Attach pull request number ⚠️ Check → Logs Raw logs
CI Sanity Check ⚠️ Check → Logs Raw logs

@@ -197,8 +195,8 @@ public Response get() {
throw new BlockingNotAllowedException();
}
try {
return c.get(readTimeoutMs, TimeUnit.MILLISECONDS);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer needed (and is actually incorrect) as the timeout handling is performed by Vert.x

Copy link
Member

@michalszynkiewicz michalszynkiewicz left a comment

Choose a reason for hiding this comment

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

it would be great to have a test for it :)

Before this change the read timeout only had an effect when doing
a synchronous call (and even then it didn't result in the proper Vert.x exception)

The connect timeout handling doesn't need any changes because it is properly handled
when org.jboss.resteasy.reactive.client.impl.ClientImpl is created

Fixes: quarkusio#17526
@geoand
Copy link
Contributor Author

geoand commented Aug 4, 2021

it would be great to have a test for it :)

Yeah, that's a good follow up :)

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 4, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 4, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building f6cd978

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build ⚠️ Check → Logs Raw logs
Attach pull request number ⚠️ Check → Logs Raw logs
CI Sanity Check ⚠️ Check → Logs Raw logs

@geoand geoand merged commit 3995936 into quarkusio:main Aug 4, 2021
@quarkus-bot quarkus-bot bot added this to the 2.2 - main milestone Aug 4, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 4, 2021
@geoand geoand deleted the #17526 branch August 4, 2021 17:08
geoand added a commit to geoand/quarkus that referenced this pull request Aug 5, 2021
geoand added a commit to geoand/quarkus that referenced this pull request Aug 5, 2021
@gsmet gsmet modified the milestones: 2.2 - main, 2.1.2.Final Aug 9, 2021
geoand added a commit that referenced this pull request Aug 19, 2021
Add test for rest-client reactive timeout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reactive REST Client does not honor readTimeout and connectTimeout
3 participants