janet-lang / spork

Various Janet utility modules - the official "Contrib" library.
MIT License
122 stars 35 forks source link

Fix parsing of messages in HTTP module #51

Closed pyrmont closed 3 years ago

pyrmont commented 3 years ago

On my system, the changes in #48 broke the tests in suite 12. It looks to me like the problem is that the changes to read-body will cause things to break if the buffer doesn't include the headers in the HTTP message. However, I think this points to a larger problem which is that the buffer should be updated after the header is read to only include the 'extra' bytes. This is the point of the buffer, after all.

To that end, this PR uses pre-pop to pop the bytes from the front of the buffer as part of the process of reading the headers. As a result of this, the 'buffer' that is passed to read-request and read-response must be a buffer (this is why the test-http-parse tests in the test suite have been changed). Because the buffer returned by read-request and read-response will not include the bytes that are in the header, it's no longer necessary to subtract the length of the bytes that are in the header from the length of the buffer in read-body.

In addition, this PR proposes some additional changes:

  1. Revert the visibility of read-http-peg. This is an internal function that abstracts the reading of header information so as to avoid code duplication across the read-request and read-response functions. I don't think it makes sense for this to be exposed to users of the library (I don't think it would be immediately clear to a user what key1 and key2 are supposed to be, for example).

  2. Rename read-http-peg to read-header. While I don't think this function should be exposed publicly for the reasons above, I do think it's unclear what it's intended to do.

I've also improved the documentation for read-request and read-response.

sogaiu commented 3 years ago

@pyrmont Not sure if it was apparent, but IIUC some bits were made public recently.

Not so sure it's good to go back on that along with renaming if some of those things were public.