Skip to content

Conversation

@graebm
Copy link
Contributor

@graebm graebm commented Dec 17, 2022

Issue:
Turns out, if aws_channel_slot_increment_read_window() was called while the "window update task" was running, the "window update task" would not get rescheduled to run again.

I was writing read-window-update tests for the python WebSocket bindings, and the test was failing as the read window approached 0. Here's how it played out:

  • the WebSocket started with a read window of 500 bytes
  • a 1000 byte payload (total frame size 1004) was sent to the WebSocket
  • the HTTP handler sent along the first 500/1004 bytes
  • the WebSocket processes this, but since the first 4 bytes were frame metadata that it doesn't count against the read window, it increments its read window by 4 bytes
  • but due to this bug, the "window update task" DOES NOT get rescheduled
  • and my test would fail because it received 4 less bytes than it expected

Description of changes:
Fix rescheduling logic for "window update task"

Rant about tests:
I lost a few hours trying to add a regression test here in aws-c-io. First, I tried adding to the existing socket_handler_echo_and_backpressure test to reproduce my issue, but it didn't fail! Turns out, the bug can only occur when you have 3+ handlers in the channel. Then I started modifying the read_write_test_handler so that it could serve as a proper "midchannel" handler that buffers up frames if necessary before sending them downstream. Then realized this would be more work complex than making a new type of test-handler that solely functions as a midchannel handler. Then realized at this point we'd be better off adding true integration tests for websocket that exercise the REAL stack of handlers, instead of a bunch of test handlers. Then realized this would take days, and I already had the new python tests I was working on that exercise the real stack of handlers... so yeah. I'll just rely on the tests I'm adding in aws-crt-python for now, but we should probably add integration tests in aws-c-http someday.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sbSteveK
Copy link
Contributor

solid name change

@graebm graebm merged commit 6c19e25 into main Dec 20, 2022
@graebm graebm deleted the sync-window-fix branch December 20, 2022 17:16
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.

2 participants