Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Include Buffer Support in the Standard Library #148

Merged
merged 21 commits into from
Apr 20, 2024
Merged

Include Buffer Support in the Standard Library #148

merged 21 commits into from
Apr 20, 2024

Conversation

CompeyDev
Copy link
Contributor

@CompeyDev CompeyDev commented Jan 21, 2024 •
edited
Loading

This PR aims to to implement support for buffers, the new luau datatype in argument and return types of functions in the lune standard library where appropriate.

TODO:

  • Rust-side handling of buffer type, in returns and params
    • @lune/fs
      • writeFile
    • @lune/net
      • FetchParams.body
      • ServeResponse.body
      • WebSocket.send
    • @lune/serde
  • Update types to reflect buffer support
  • Update unit tests to test buffer handling

Closes #137.

@CompeyDev
feat: buffer support for @lune/fs
caecf9e 
* fs.readFile now returns a buffer, instead of a string
* fs.writeFile now accepts a string or a buffer for the contents
  argument
@CompeyDev
feat(types + tests): updated types and tests for @lune/fs
a1b1134
@CompeyDev
feat(util + net): common buffer util & buffer support for @lune/net
746b1cc 
This commit implements buffer types are arguments and returns for
previously used string types for the @lune/net library.

Methods affected:
* net.request - Returns resp body as a buffer now
* net.serve - RequestConfig handlers can have a body of type string or
  buffer now
* net.socket - WebSocket stream data can now be sent as a string or
  buffer
@CompeyDev
feat(types): update @lune/net types to reflect buffer support
185fb6a
@CompeyDev CompeyDev marked this pull request as draft January 21, 2024 09:19
@CompeyDev CompeyDev mentioned this pull request Feb 21, 2024
Closed
@CompeyDev
Copy link
Contributor Author

@filiptibell Could we decide on a design for this? Looking forward to getting this implemented soon, buffers aren't very useful inside lune atm.

@filiptibell
Copy link
Collaborator

I think we can start with just accepting a string | buffer any place that the builtins currently accept string and is converting to bytes. Changing other APIs to return buffers instead of strings is going to require more sweeping changes and maybe a lune 0.9.0 version bump for breaking changes, so if we want to get buffer support into peoples hands quicker its best to leave for another PR. We do have buffer.fromstring after all.

Right now we don't have a performant way of converting to/from buffer userdata values though - making this more or less useless since performance is the main benefit of buffers - I have opened an issue for this at mlua-rs/mlua#378

@filiptibell filiptibell added the external Issue depends on external factors label Mar 11, 2024
@CompeyDev
Copy link
Contributor Author

That clarifies a bunch now, guess I can change the PR up to do the above. Although I do think we should default to returning buffers for APIs sometime in the future (I've noticed that most runtimes like NodeJS do this for fs reads and similar).

@filiptibell
Copy link
Collaborator

Latest mlua version now has some buffer support that we could use by accepting BString in rust functions/methods where applicable 🚀

@CompeyDev
Copy link
Contributor Author

Awesome! I'll get started on this sometime soon, in that case :)

@CompeyDev
Copy link
Contributor Author

Since this is now a part of the 0.9.0 milestone, I'll implement the breaking changes as well.

@filiptibell
Copy link
Collaborator

Since this is now a part of the 0.9.0 milestone, I'll implement the breaking changes as well.

We still don't know which APIs should change and how, or if it should be separate options to return buffers.
Please keep the scope of this PR to just accepting buffers as arguments

@CompeyDev CompeyDev marked this pull request as ready for review April 20, 2024 11:38
filiptibell
filiptibell requested changes Apr 20, 2024
View reviewed changes
Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a typedef that shouldn't have changed but otherwise LGTM 👍

types/net.luau Outdated Show resolved Hide resolved
@CompeyDev @filiptibell
chore(types): revert inaccurate type
2000cca 
Co-authored-by: Filip Tibell <filip.tibell@gmail.com>
@filiptibell filiptibell merged commit f830ce7 into lune-org:main Apr 20, 2024
6 checks passed
@filiptibell
Copy link
Collaborator

Seems like serde tests for compression are failing after merging this.. I'm a bit confused as to why, the tests passed in the PR itself, and the test files have not changed according to git either

@CompeyDev
Copy link
Contributor Author

That's... odd. I'll investigate and lyk.

@CompeyDev
Copy link
Contributor Author

CompeyDev commented Apr 21, 2024 •
edited
Loading

Tests seem to pass for f830ce7, but not on the latest commit - something must've gone wrong between the two.

EDIT: Looks like 70f0c55 is the first commit where the tests start to fail.

@CompeyDev
Copy link
Contributor Author

CompeyDev commented May 10, 2024 •
edited
Loading

After compressing loremipsum.txt with the latest version of GNU GZIP, the test case almost passes, except for the fact that it includes 15 additional characters - the name of the file being compressed. Previously, the compressed and target file had completely unrelated contents.

image

Using -n disables including the original filename and timestamp, but a singular byte is different in the target and produced output.

@filiptibell
Copy link
Collaborator

I've fixed up all of the test files and discrepancies. I'm leaving a mini-postmortem here for future reference and anyone interested:

  1. We upgraded our dependencies.
  2. Gzip is not guaranteed to be stable in its representation other than the header - and zlib even less so, since it is actually the same compression algorithm but without a container. Tests started failing because this underlying implementation for zlib compression changed.
  3. I no longer had a history of how the reference files for compression were generated, so I wrote this script so that we will be able to use to have a reference implementation that generates all test files using (GNU where possible) cli utilities.
  4. The 1 byte gzip difference is because of the "os" field in the gzip header. Lune intentionally sets this as "unknown" on all platforms. Generating using command-line tools will set this to an os-specific value. The new test files generation script overwrites this to be "unknown" after compression using the cli utilities.

After figuring out parameters for each compression cli and re-generating the test files using this much more deterministic method, tests started to pass....except for lz4. It turns out lz4-flex does not support the high compression mode of lz4 at all, so it failed on any output from the lz4 cli. I've migrated to using the lz4 crate which is a bit slower but much better for compatibility.

It may be worth considering if we should test for exact matches in output like this at all, since the output may vary depending on the zlib implementation used, but I think I at least left the tests in a much better state. If this happens again we can just run the test file generation again and verify that things are still ok.

@BrewTestBot BrewTestBot mentioned this pull request May 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filiptibell filiptibell filiptibell approved these changes

Assignees
No one assigned
Labels
external Issue depends on external factors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for buffers in built-in libraries
2 participants
@CompeyDev @filiptibell