Conversation
|
I've read this document and can understand it, but I do not attest to its accuracy. Please get a further review before merging. |
|
@tylerkaraszewski Could I get your eyes/thoughts on this please? 🙏 |
|
|
||
| This document describes the protocol as deployed. Section 15 lists behaviour | ||
| that appears to be defective; that behaviour is documented but is not normative, | ||
| and implementations should not rely on it. |
There was a problem hiding this comment.
Weird statement, as if we're writing a spec for a standard that multiple implementers will build.
There was a problem hiding this comment.
Fair. Reworded to drop the multiple-implementers framing:
"Section 15 lists behaviour that appears to be defective. That behaviour is recorded so it is not mistaken for design, and is subject to change without notice."
| 2. **A blank line ends the fields.** Everything after it is the body. | ||
| 3. **`Content-Length` gives the body length in octets.** Send it, and make it | ||
| exact, on any connection you will reuse. See section 4.4; this is the rule | ||
| most often got wrong. |
There was a problem hiding this comment.
List examples where this was often gotten wrong, please.
If I were making this an actual spec, I would just make Content-Length mandatory, even though we accept requests that lack it (which is useful for handwritten commands and such)
There was a problem hiding this comment.
Took your suggestion instead: Content-Length is now mandatory in section 4.4. A sender MUST send it and it MUST be exact; a receiver MUST accept its absence so hand-written commands still work. That deleted the section I'd built to explain the ambiguity.
| 6. **Connections are persistent.** Send `Connection: close` when finished, or | ||
| the server waits for another request. | ||
|
|
||
| Two things that surprise newcomers: |
There was a problem hiding this comment.
Who was surprised? Come on, did you actually read this? I question the value of using AI to generate a document that nobody will read and will quickly become out-of-date over just asking AI to freshly answer a specific question about the code based on the current state of the code when the question is asked.
And if we want this for some sort of compliance reason get the AI to write it as an RFC type document without the editorializing.
I am stopping with the review for now.
There was a problem hiding this comment.
Removed, along with seven more instances you hadn't reached: "commonly misunderstood", "In practice", "the notable exception", "the subtle one", and others. The doc no longer asserts anything about readers.
On staleness: added test/tests/ProtocolDocTest.cpp. It feeds all 16 example messages through SParseHTTP and fails the build if any stops matching. Verified it catches a Content-Length off by one in either direction.
That covers the examples, not the prose. If it's not enough, the alternatives are cutting this to sections 3-8 (wire format only) or closing it.
There was a problem hiding this comment.
Data from this PR:
- Five of seven example
Content-Lengthvalues were wrong. 68 vs 55, 24 vs 19, 62 vs 52, 178 vs 218, and one off by a single octet, 34 vs 33. The two correct ones had been copied fromdocs/index.md. - More than a dozen of the 182
file:linecitations pointed at the wrong line. Now replaced with function names. - Two sections contradicted each other. 6.6 said a sender's
Content-Lengthis discarded; 7.1 said a sender MUST send it. Both true of different messages, nonsense together.
All caught in review. That's the difference: this got two reviewers and a test. An on-demand answer gets neither, and a wrong byte count reads exactly like a right one.
It also found two real defects, both in section 15: Content-Length is converted with no validation and the failure escapes the peer error handler, and chunked parsing hardcodes a two-octet terminator so a bare LF loses a byte.
The test only checks the examples. It does not stop the rest of the doc going stale.
The document asserted things it had no basis for: which rule is 'most often got wrong', what 'surprises newcomers', and which case is 'the subtle one'. State the protocol and let the reader judge.
A sender MUST now send Content-Length on every message, and it MUST be exact. A receiver still MUST accept its absence, so a command can be typed by hand. This replaces the section that documented the ambiguity instead of resolving it.
Parses all 16 example messages in docs/protocol.md with SParseHTTP, so an example that stops matching the protocol fails the build. Markdown cannot represent a trailing newline or a CRLF inside a fence, so each example fence carries an info string saying how to rebuild its body: bwp, bwp-lf, or bwp-crlf.
This branch has not been deployed
3 participants
Initial documentation of the Bedrock Wire Protocol specification.