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

util: Fix call_all hang when stream is pending #656

Merged
merged 2 commits into from Mar 29, 2022

Conversation

leoyvens
Copy link
Contributor

Currently call_all will hang in a busy loop if called when the input stream is pending.

@davidpdrsn davidpdrsn requested review from hawkw and olix0r March 28, 2022 10:31
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this looks good to me overall, thanks for the fix!

i commented on one style suggestion, but it's not a blocker.

@@ -111,6 +111,7 @@ where
Poll::Pending => {
// TODO: We probably want to "release" the slot we reserved in Svc here.
// It may be a while until we get around to actually using it.
return Poll::Pending;
Copy link
Member

Choose a reason for hiding this comment

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

nit, take it or leave it: now that we return Poll::Pending in this case, we could simplify the nested match to just a single match by using ready! instead:

            // If it is, gather the next request (if there is one), or return `Pending` if the
            // stream is not ready.
            // TODO: We probably want to "release" the slot we reserved in Svc if the
            // stream returns `Pending`. It may be a while until we get around to actually
            // using it.
            match ready!(this.stream.as_mut().poll_next(cx)) {
                Some(req) => {
                    this.queue.push(svc.call(req));
                }
                None => {
                    // We're all done once any outstanding requests have completed
                    *this.eof = true;
                }
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review, I've done this refactor.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks great, thank you!

@hawkw hawkw enabled auto-merge (squash) March 29, 2022 17:42
auto-merge was automatically disabled March 29, 2022 18:17

Head branch was pushed to by a user without write access

@leoyvens leoyvens force-pushed the fix-call-all-pending-stream branch from b6f403d to b8f0301 Compare March 29, 2022 18:17
@leoyvens
Copy link
Contributor Author

@hawkw Thanks for the review! I forced push so this still needs a merge.

@hawkw hawkw merged commit cc9e5be into tower-rs:master Mar 29, 2022
hawkw pushed a commit that referenced this pull request Jun 17, 2022
Currently `call_all` will hang in a busy loop if called when the input
stream is pending.
hawkw pushed a commit that referenced this pull request Jun 17, 2022
Currently `call_all` will hang in a busy loop if called when the input
stream is pending.
hawkw added a commit that referenced this pull request Jun 17, 2022
# 0.4.13 (June 17, 2022)

### Added

- **load_shed**: Public constructor for `Overloaded` error ([#661])

### Fixed

- **util**: Fix hang with `call_all` when the `Stream` of requests is
  pending ([#656])
- **ready_cache**: Ensure cancelation is observed by pending services
  ([#668], fixes [#415])
- **docs**: Fix a missing section header due to a typo ([#646])
- **docs**: Fix broken links to `Service` trait ([#659])

[#661]: #661
[#656]: #656
[#668]: #668
[#415]: #415
[#646]: #646
[#659]: #659
hawkw added a commit that referenced this pull request Jun 17, 2022
# 0.4.13 (June 17, 2022)

### Added

- **load_shed**: Public constructor for `Overloaded` error ([#661])

### Fixed

- **util**: Fix hang with `call_all` when the `Stream` of requests is
  pending ([#656])
- **ready_cache**: Ensure cancelation is observed by pending services
  ([#668], fixes [#415])
- **docs**: Fix a missing section header due to a typo ([#646])
- **docs**: Fix broken links to `Service` trait ([#659])

[#661]: #661
[#656]: #656
[#668]: #668
[#415]: #415
[#646]: #646
[#659]: #659
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

2 participants