Skip to content

False timeouts when using StreamedAsyncHandler #1721

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

Closed
n-miles opened this issue May 5, 2020 · 5 comments · Fixed by #1723
Closed

False timeouts when using StreamedAsyncHandler #1721

n-miles opened this issue May 5, 2020 · 5 comments · Fixed by #1723

Comments

@n-miles
Copy link
Contributor

n-miles commented May 5, 2020

When using the StreamedAsyncHandler to download large files, I found what I think is a bug in how timeouts are handled.

The timeout mechanism relies on when the ListenableFuture for the request was last touch()ed. touch() is only called when a HttpResponseBodyPart is delivered to the downstream. This works fine if the downstream is downloading the response as fast as possible.

However, if the downstream has no outstanding request() (like if it's serving as a proxy and its own downstream is slow or whatever), there may be long periods of time where there are no body parts published because none have been requested. In those periods, the request may "time out" because of inactivity.

I think we should change the timeout logic to only count this as a timeout if there has been no activity AND there is no outstanding request on the Publisher. This can be implemented by periodically touch()ing the ListenableFuture if there is no outstanding request() on the Publisher.

I am willing to implement it if we want this feature.

@slandelle
Copy link
Contributor

I am willing to implement it if we want this feature.

Sure.

@n-miles
Copy link
Contributor Author

n-miles commented May 5, 2020

@slandelle do you have any concerns about backwards compatibility or should I just add it to the existing StreamedAsyncHandler logic?

@slandelle
Copy link
Contributor

do you have any concerns about backwards compatibility

Not on the reactive-streams features. Those are quite experimental (originally contributed by Lightbend, then nobody ever answered my questions).

@n-miles
Copy link
Contributor Author

n-miles commented May 8, 2020

Opened #1723

@n-miles
Copy link
Contributor Author

n-miles commented Jun 4, 2020

@slandelle when will this change make it into a release?

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 a pull request may close this issue.

2 participants