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.

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

implement brotli classes #2619

Merged
merged 1 commit into from
Aug 30, 2024
Merged

implement brotli classes #2619

merged 1 commit into from
Aug 30, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Aug 27, 2024

Implements:

  • new BrotliCompress()
  • new BrotliDecompress()
  • createBrotliDecompress()
  • createBrotliCompress()
    functions and classes to node:zlib

@anonrig anonrig force-pushed the yagiz/add-brotli-classes branch 16 times, most recently from 17cc7e0 to 4d2dfbc Compare August 30, 2024 17:31
@anonrig anonrig requested review from jasnell and npaun August 30, 2024 17:32
@anonrig anonrig marked this pull request as ready for review August 30, 2024 17:32
@anonrig anonrig requested review from a team as code owners August 30, 2024 17:32
@anonrig anonrig mentioned this pull request Aug 30, 2024
21 tasks
@anonrig anonrig requested a review from fhanau August 30, 2024 17:53
@npaun
Copy link
Member

npaun commented Aug 30, 2024

GH won't let me comment on the line, but turn on the brotli tests in zlibEmptyBuffer

@anonrig
Copy link
Member Author

anonrig commented Aug 30, 2024

GH won't let me comment on the line, but turn on the brotli tests in zlibEmptyBuffer

@npaun Those are the one-shot methods that this pull-request doesn't cover.

src/workerd/api/node/zlib-util.c++ Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.h Show resolved Hide resolved
@npaun
Copy link
Member

npaun commented Aug 30, 2024

@anonrig Right, that's my job then hehe

@npaun
Copy link
Member

npaun commented Aug 30, 2024

Just flagging that there's a memory issue in the linux-asan build

static jsg::Ref<ZlibStream> constructor(ZlibModeValue mode);
explicit CompressionStream(ZlibMode _mode): context_(_mode) {}
CompressionStream() = default;
~CompressionStream();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
~CompressionStream();
~CompressionStream() noexcept(false);

Copy link
Member Author

Choose a reason for hiding this comment

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

Not possible:

src/workerd/api/node/zlib-util.h:260:5: error: exception specification is not available until end of class definition
  260 |     ~CompressionStream() noexcept(false);

@anonrig anonrig force-pushed the yagiz/add-brotli-classes branch 2 times, most recently from 7a31c11 to faa4e7c Compare August 30, 2024 20:00
Copy link
Member

@npaun npaun left a comment

Choose a reason for hiding this comment

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

LGTM but I'd wait for James || Felix to approve as well before merging because it's a bigger change

@anonrig anonrig force-pushed the yagiz/add-brotli-classes branch 2 times, most recently from 15722d1 to 728c87d Compare August 30, 2024 20:44
src/workerd/api/node/zlib-util.c++ Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Show resolved Hide resolved
}

protected:
CompressionContext* context() {
Copy link
Member

@jasnell jasnell Aug 30, 2024

Choose a reason for hiding this comment

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

Non-blocking: Our typical style would be to return a CompressionContext& here instead... and if you do want/need to provide the pointer, provide a separate CompressionContext* get() or something.

private:
// Used to store allocations in Brotli* operations.
// This declaration should be physically positioned before
// context to avoid `heap-use-after-free` ASan error.
Copy link
Member

Choose a reason for hiding this comment

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

non-blocking nit: I would expand the comment slight to connect this more concretely with the AllocFor_ and FreeFor_ APIs above.

@jasnell jasnell merged commit 2482d43 into main Aug 30, 2024
13 of 14 checks passed
@jasnell jasnell deleted the yagiz/add-brotli-classes branch August 30, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants