Only "data" frames affect websocket's read window now #407
Conversation
Codecov ReportBase: 79.24% // Head: 79.19% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## websocket-changes #407 +/- ##
=====================================================
- Coverage 79.24% 79.19% -0.06%
=====================================================
Files 27 27
Lines 11752 11761 +9
=====================================================
+ Hits 9313 9314 +1
- Misses 2439 2447 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
TingDaoK
left a comment
TingDaoK
left a comment
There was a problem hiding this comment.
Looks good, just trivial stuff.
| * Required. | ||
| * Initial size of the websocket's read window. | ||
| * Ignored unless `manual_window_management` is true. | ||
| * Set to 0 to prevent any incoming websocket frames until aws_websocket_increment_read_window() is called. |
There was a problem hiding this comment.
Whenever the read window reaches 0, you will stop receiving data.
Set to 0 to prevent any incoming websocket frames until aws_websocket_increment_read_window() is called.
So, the window is for "data" frames only, but if it reaches to 0, will "control" frames be affected?
I think PING/PONG and CLOSE should not be affected? But, as we use tcp window, maybe we are not able to do that?
There was a problem hiding this comment.
if the window gets to 0, everything stops
I updated the comments to elaborate how the websocket still auto-increments when it gets any other types of bytes, but once you hit 0, everything stops
include/aws/http/websocket.h
Outdated
| * The payload data is always unmasked at this point. | ||
| * | ||
| * NOTE: If you created the websocket with `manual_window_management` set true, you must maintain the read window. | ||
| * Whenever the read window reaches 0, you will stop receiving data. |
There was a problem hiding this comment.
you will stop receiving data
Data here is unclear to me. Like only data frame, or all possible data?
There was a problem hiding this comment.
hopefully more clear now?
3 participants
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.