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.

Sign up for GitHub

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

Jump to bottom

HTTP/2 UniformStreamByteDistributor can get into an infinite loop #4588

Closed
Scottmitch opened this issue Dec 17, 2015 · 5 comments
Closed

HTTP/2 UniformStreamByteDistributor can get into an infinite loop #4588

Scottmitch opened this issue Dec 17, 2015 · 5 comments
Assignees
@Scottmitch
Labels
defect
Milestone
4.1.0.CR1

Comments

@Scottmitch
Copy link
Member

UniformStreamByteDistributor loops over a queue until empty. It is possible for a stream state to continuously be added to a the queue and cause this loop to continue indefinitely.

A state is popped off the queue, and written. During that write process the updateStreamableBytes method is called by the flow controller. If hasFrame() returns true, then the state is enqueued. If the connection window is larger than the stream window, it is possible that the stream's window has become collapsed such that streamableBytes goes to 0 but hasFrame() will return true. An exit criteria on the loop is to check for maxBytes transitioning to zero, but this may never happen if the streams can not consume all of the connection window, and so the loop will never terminate.

This was discovered as part of the fix for #4587.
@Scottmitch Scottmitch self-assigned this Dec 17, 2015
@Scottmitch
Copy link
Member Author

I ran into this issue while attempting to use UniformStreamByteDistributor in DefaultHttp2RemoteFlowControllerTest...which was done as part of the PR for #4587.

@Scottmitch Scottmitch added this to the 4.1.0.CR1 milestone Dec 17, 2015
@Scottmitch
Copy link
Member Author

@nmittler - FYI

@nmittler
Copy link
Member

@Scottmitch good find! Do you have a PR pending?

@Scottmitch Scottmitch mentioned this issue Dec 18, 2015
Closed
@Scottmitch
Copy link
Member Author

@nmittler - Yes. #4599.

Scottmitch added a commit to Scottmitch/netty that referenced this issue Dec 18, 2015
@Scottmitch
HTTP/2 DefaultHttp2RemoteFlowController Stream writability notificati…
3e02cb6 
…on broken

Motivation:
DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor no longer reliably detects when a stream's writability change occurs.

Modifications:
- Ensure writiability is reliabily reported by DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor
- Fix infinite loop issue (netty#4588) detected when consolidating unit tests

Result:
Reliable stream writability change notification, and 1 less infinite loop in UniformStreamByteDistributor.
Fixes netty#4587
Scottmitch added a commit that referenced this issue Dec 21, 2015
@Scottmitch @normanmaurer
HTTP/2 DefaultHttp2RemoteFlowController Stream writability notificati…
9ac430f 
…on broken

Motivation:
DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor no longer reliably detects when a stream's writability change occurs.

Modifications:
- Ensure writiability is reliabily reported by DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor
- Fix infinite loop issue (#4588) detected when consolidating unit tests

Result:
Reliable stream writability change notification, and 1 less infinite loop in UniformStreamByteDistributor.
Fixes #4587
@Scottmitch
Copy link
Member Author

Resolved with #4599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Scottmitch Scottmitch

Labels
defect
Projects
None yet
Milestone
4.1.0.CR1
Development

No branches or pull requests
2 participants
@nmittler @Scottmitch