jow- / ucode

JavaScript-like language with optional templating
ISC License
88 stars 27 forks source link

Zlib: add stream-oriented operations #212

Open f00b4r0 opened 1 month ago

f00b4r0 commented 1 month ago

This PR introduces stream-oriented primitives for dealing with zlib data.

The code is split into functional commits for easier review.

Memory pressure is directly dependent on how often the stream output buffer is flushed through read() calls, so this implementation makes it possible to continuously compress data within a memory cap.

Notes:

Only happy testing has been performed (successfully) so far, comments welcome

HTH

jow- commented 1 month ago

On a first glance this looks good. Not overly happy with the constructor naming, "infnew" and "defnew" reminds me on infinity and default, not inflate and deflate for some reason. What about calling the constructors inflater() and deflater() or inflate_stream() and deflate_stream() ?

f00b4r0 commented 1 month ago

On a first glance this looks good. Not overly happy with the constructor naming, "infnew" and "defnew" reminds me on infinity and default, not inflate and deflate for some reason. What about calling the constructors inflater() and deflater() or inflate_stream() and deflate_stream() ?

Ok, I'll go with the former to keep things short.

  • Rename the internal resource type names to zlib.inflate and zlib.deflate for clarity

Won't that be confusing with the same-name functions ?

  • Is Z_FINISH really a useful default flush mode for the deflate write() method? Should we maybe default it to Z_NO_FLUSH?

  • Dito for inflate write()

I suppose you have a point. My initial idea was to have the stream functions default to the same behavior as the single-call oriented ones, and force the user to select a flush mode, but it's probably not ideal now that I think about it. Will switch to Z_NO_FLUSH instead.

  • The documentation is a bit broken, you should not put @see text inline into text (to test documentation rendering locally, run npm install; npm run doc in the source dir, then browse docs/module-zlib.html)

Sorry no can do. I don't have npm on my system and no intention of installing it just for this use given how it's been a proven attack vector multiple times already. Sorry. I'll fix the @see.

  • The call stream.write('', Z_FINISH) looks a bit messy, maybe it makes sense to implement a stream.finish() convenience method?

Actually the example is clumsy (I was lazy there). In real life you would of course use Z_FINISH with the last sent chunk, not with an empty one. The latter works and completes the stream to a valid archive but adds an unnecessary extra block. I'll improve the doc.

jow- commented 1 month ago

Won't that be confusing with the same-name functions ?

No, that string literal for the resource type is just used to uniquely identify the resource type within the global VM context, it does not share the namespace with variables. The name is also used when converting resource values to strings, e.g. for printing. A deflate stream will appear as something like <zlib.strmd 0xdeadbeef>. To make that name more descriptive, I'd suggest zlib.deflate or zlib.dstream or zlib.deflateStream. Compare with the resource labels in other modules: