fix: add netstring encode/decode/stream library #1808
Conversation
|
For TChannel, we used a bounded search test to suss out boundary conditions in the framing protocol. This should be easier. https://github.com/uber/tchannel-node/blob/master/test/streaming_bisect.js |
| assert.equal(encoding, 'buffer'); | ||
| buffered = Buffer.concat([buffered, chunk]); | ||
| let res = null; | ||
| try { |
There was a problem hiding this comment.
Node.js deoptimizes any function contain try/catch. This can be addressed by layering any function that contains try/catch such that it’s isolated in a very small inner frame.
There was a problem hiding this comment.
does that mean minimizing the number of variables closed over by the function containing the try/catch?
There was a problem hiding this comment.
No, minimizing the closure isn’t necessary.
| eq('abc', '3:abc,'); | ||
| const umlaut = 'ümlaut'; | ||
| t.is(umlaut.length, 6); | ||
| const umlautBuffer = Buffer.from(umlaut, 'utf-8'); |
There was a problem hiding this comment.
I’ve been leaning harder on web API’s to make these kinds of libraries easier to port. To that end, Uint8Array, TextDecoder, and TextEncoder are handy, albeit less convenient than Buffer.from.
There was a problem hiding this comment.
It would also increase portability to frame this API in terms of async iterators and relieve the dependence on Node.js-specific streams. Node.js streams do implement async iterators so bridging should not be hard.
| } | ||
|
|
||
| // Input is a Buffer containing zero or more netstrings and maybe some | ||
| // leftover bytes. Output is zero or more decoded Buffers, one per netstring, |
There was a problem hiding this comment.
Might be clearer if s/decoded buffers/strings.
There was a problem hiding this comment.
but I don't think they are strings, right? they're strictly Buffer objects, if I understand correctly
There was a problem hiding this comment.
Ah, you are right. I was confused by the symmetry and “decoded”. Would “extracted” be a better term? Decode implied bytes in, string out to me.
warner
force-pushed
the
1807-netstring
branch
from
6e21c54
to
fd1da9e
Compare
|
Ok, I got the code all working, and expanded the tests (and also squashed, sorry). Ready for proper review now. I'm uncertain about the Buffer/string distinction; I agree that using U8IntArray would be more general, but it sounds like it might be slightly harder to use, and our use case is somewhat narrow (two functions, both under our control, both in a Node.js -based start-compartment). I'll follow your judgement, do you think I should change it from what it looks like now? |
| } | ||
|
|
||
| // Input is a Buffer containing zero or more netstrings and maybe some | ||
| // leftover bytes. Output is zero or more decoded Buffers, one per netstring, |
There was a problem hiding this comment.
Ah, you are right. I was confused by the symmetry and “decoded”. Would “extracted” be a better term? Decode implied bytes in, string out to me.
| assert.equal(encoding, 'buffer'); | ||
| buffered = Buffer.concat([buffered, chunk]); | ||
| let res = null; | ||
| try { |
There was a problem hiding this comment.
No, minimizing the closure isn’t necessary.
|
No further changes necessary for portability or performance in this first cut. |
For use by the kernel-worker protocol.
refs #1797
closes #1807
This is not yet complete: I still need tests for the streaming interfaces, which I'm not entirely sure how to write.
@kriskowal could you eyeball the implementation and maybe see if I'm on the right track? Any ideas about how I should go about the test?