Conversation
|
Analysis of PR #22714 at 3d62f2a6: Test 641 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Test 642 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Generated by Testclutch |
There was a problem hiding this comment.
Pull request overview
Fixes partial Gopher request terminator sends.
Changes:
- Adds a reusable complete-buffer send loop.
- Uses it for selectors and CRLF terminators.
- Adds a Gopher header-dump test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/gopher.c |
Handles partial selector and CRLF sends. |
tests/data/test5028 |
Tests client-visible Gopher headers. |
tests/data/Makefile.am |
Registers the new test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Extract the buffer-sending logic into its own function so it can also be used when sending CRLF. Previously, CRLF could be partially sent to the server without reporting an error or sending the remaining bytes. This commit preserves how the old code handled errors while writing CRLF to the client, which is why the extracted function has a bool parameter. This also includes a test case that verifies that the header written to the client contains exactly one CRLF. This test case also detects the old buggy behaviour of partial sends.
de9ab10 to
41d6901
Compare
|
Hi, I applied the suggested fix (and added some context about it in the test file), clarified a comment in the Gopher implementation, and squashed the commits. |
|
Thanks! |
I was reading the Gopher implementation and noticed that
nwrittenis never checked after sending the CRLF to the server. Reading here and there left me with the impression that this could lead to partial sends of the CRLF. The commit body better describes the changes.