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

add several node:zlib classes #2519

Merged
merged 3 commits into from
Aug 22, 2024
Merged

add several node:zlib classes #2519

merged 3 commits into from
Aug 22, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Aug 12, 2024

Lacks tests but before adding more, I think this is getting ready to have an additional pair of eyes.

Remaining todo:

  • Add tests
  • Investigate why eslint is causing so much trouble (hence eslint-ignore)

@anonrig anonrig force-pushed the yagiz/zlib-gzip branch 3 times, most recently from ced872e to 18a70ba Compare August 13, 2024 19:50
src/workerd/api/node/zlib-util.h Show resolved Hide resolved
src/workerd/api/node/zlib-util.h Outdated Show resolved Hide resolved
src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/zlib-gzip branch 25 times, most recently from 1a6329f to fff3e5c Compare August 19, 2024 01:21
@anonrig anonrig changed the title [WIP] add zlib.Gzip class add several node:zlib classes Aug 19, 2024
@anonrig anonrig requested a review from fhanau August 20, 2024 19:52
@anonrig anonrig force-pushed the yagiz/zlib-gzip branch 3 times, most recently from 7870ef1 to 21868f5 Compare August 20, 2024 23:09
@anonrig anonrig requested review from jasnell and npaun August 21, 2024 15:09
src/node/zlib.ts Outdated Show resolved Hide resolved
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Looks good once we're sure the internal CI is green.

src/node/zlib.ts Outdated Show resolved Hide resolved
src/workerd/io/compatibility-date.capnp Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/zlib-gzip branch 2 times, most recently from ff27190 to e3fe79c Compare August 21, 2024 15:42
Copy link
Collaborator

@mikea mikea left a comment

Choose a reason for hiding this comment

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

appreciate the effort to make all .ts code typed.

src/workerd/api/node/zlib-util.h Outdated Show resolved Hide resolved
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.

Tests are failing under ASan. I'm worried there might be some kind of corruption.

@anonrig anonrig requested a review from npaun August 21, 2024 18:06
@npaun
Copy link
Member

npaun commented Aug 21, 2024

Do we know why the unnecessary mutex guard caused the test failure?

It doesn't immediately jump out at me what this has to do with a mutex
Fatal uncaught kj::Exception: workerd/api/node/zlib-util.c++:311: failed: expected status == 0 || status == (-3); jsg.Error: Uncaught error on closing zlib stream

@anonrig
Copy link
Member Author

anonrig commented Aug 21, 2024

Do we know why the unnecessary mutex guard caused the test failure?

Yes, the class destructor was checking the reference not the value, and causing the sanitizer to fail.

@npaun
Copy link
Member

npaun commented Aug 21, 2024

Aside: That implicit conversion to operator T* in libkj is kind of wack.

wack

@anonrig anonrig merged commit 394541f into main Aug 22, 2024
10 of 11 checks passed
@anonrig anonrig deleted the yagiz/zlib-gzip branch August 22, 2024 15:18
@npaun npaun mentioned this pull request Aug 22, 2024
21 tasks
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.

4 participants