Document and pin existing behavior - #578
Open
azkrishpy wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
N/A. Part of the
todoschore stack (#576), stacked on #577.Description of changes:
Resolves five TODO comments that were asking for documentation or for tests that were never written. No behavior change — every change here is a comment, or a test that pins behavior which already exists.
Documented existing behavior
websocket.h— replaced the two "document lifetime stuff" / "document CLOSE frame behavior" TODOs with the actual rules: the websocket is ref-counted,aws_websocket_release()is not a close, and a received CLOSE stops reading but does not close the connection. The remainingaws_input_streamTODO is a feature request and is left alone.strutil.h/strutil.c—aws_strutil_is_http_request_target()asked for the full RFC7230 5.3 / RFC3986 grammar. It is deliberately not implemented: the strict grammar rejects request-targets that servers accept in practice. Recorded what the function actually guarantees (rejects non-visible ASCII, which is what could split the request line) instead of leaving it as an open request.connection_manager.c— "seems like connections can still be released while shutting down" is correct, and is required:vended_connection_countonly reaches zero via release.h1_connection.h— dropped a stale "introduce naming conventions" TODO; the surrounding functions already follow theaws_h1_connection_*convention.Enabled two dormant tests
tests/CMakeLists.txthad three# TODO add_test_case(...)lines for window-update cases that were never implemented. Implemented two of them:h2_client_manual_updated_window_ignored_when_automatical_on—aws_http_stream_update_window()is ignored when manual window management is off, so no second WINDOW_UPDATE is sent on top of the automatic one.h2_client_manual_stream_updated_window_ignored_invalid_state— a window update on a stream that has not been activated is dropped, since the stream has no id on the wire yet.The third,
h2_client_manual_window_management_window_overflow, is already covered by the existing*_overflow_cappedcases directly above it, so it is a comment pointing there rather than a duplicate test.test_strutil.calso had the same grammar TODO in place of assertions. Replaced it with cases covering both sides of the contract: all non-visible ASCII is rejected, while braces, pipes, quotes, carets, malformed percent-encoding and fragments are accepted.Verification
747 tests pass, 0 failures (745 before — the two newly enabled cases).
format-check.pyclean.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.