From c235f002efd4cf5feddc850f543b30d44af87b9c Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 19 Jun 2024 20:11:22 +0530 Subject: [PATCH 1/3] fix(ext/node): use cppgc for node:zlib Fixes https://github.com/denoland/deno/issues/22470 --- ext/node/lib.rs | 1 - ext/node/ops/zlib/mod.rs | 98 +++++++++------------------- ext/node/polyfills/_zlib_binding.mjs | 27 ++++---- tests/unit_node/zlib_test.ts | 10 +-- 4 files changed, 47 insertions(+), 89 deletions(-) diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 7654607d7af008..d0a8e24d5206ee 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -332,7 +332,6 @@ deno_core::extension!(deno_node, ops::zlib::op_zlib_close, ops::zlib::op_zlib_close_if_pending, ops::zlib::op_zlib_write, - ops::zlib::op_zlib_write_async, ops::zlib::op_zlib_init, ops::zlib::op_zlib_reset, ops::zlib::brotli::op_brotli_compress, diff --git a/ext/node/ops/zlib/mod.rs b/ext/node/ops/zlib/mod.rs index 55a2e58638c93e..a17eb30b2711b6 100644 --- a/ext/node/ops/zlib/mod.rs +++ b/ext/node/ops/zlib/mod.rs @@ -242,7 +242,7 @@ impl ZlibInner { } struct Zlib { - inner: RefCell, + inner: RefCell>, } impl deno_core::Resource for Zlib { @@ -251,12 +251,9 @@ impl deno_core::Resource for Zlib { } } -#[op2(fast)] -#[smi] -pub fn op_zlib_new( - state: &mut OpState, - #[smi] mode: i32, -) -> Result { +#[op2] +#[cppgc] +pub fn op_zlib_new(#[smi] mode: i32) -> Result { let mode = Mode::try_from(mode)?; let inner = ZlibInner { @@ -264,18 +261,17 @@ pub fn op_zlib_new( ..Default::default() }; - Ok(state.resource_table.add(Zlib { - inner: RefCell::new(inner), - })) + Ok(Zlib { + inner: RefCell::new(Some(inner)), + }) } #[op2(fast)] -pub fn op_zlib_close( - state: &mut OpState, - #[smi] handle: u32, -) -> Result<(), AnyError> { - let resource = zlib(state, handle)?; - let mut zlib = resource.inner.borrow_mut(); +pub fn op_zlib_close(#[cppgc] resource: &Zlib) -> Result<(), AnyError> { + let mut resource = resource.inner.borrow_mut(); + let mut zlib = resource + .as_mut() + .ok_or_else(|| type_error("zlib not initialized"))?; // If there is a pending write, defer the close until the write is done. zlib.close()?; @@ -283,44 +279,11 @@ pub fn op_zlib_close( Ok(()) } -#[allow(clippy::too_many_arguments)] -#[op2(async)] -#[serde] -pub fn op_zlib_write_async( - state: Rc>, - #[smi] handle: u32, - #[smi] flush: i32, - #[buffer] input: &[u8], - #[smi] in_off: u32, - #[smi] in_len: u32, - #[buffer] out: &mut [u8], - #[smi] out_off: u32, - #[smi] out_len: u32, -) -> Result>, AnyError> { - let mut state_mut = state.borrow_mut(); - let resource = zlib(&mut state_mut, handle)?; - - let mut strm = resource.inner.borrow_mut(); - let flush = Flush::try_from(flush)?; - strm.start_write(input, in_off, in_len, out, out_off, out_len, flush)?; - - let state = state.clone(); - Ok(async move { - let mut state_mut = state.borrow_mut(); - let resource = zlib(&mut state_mut, handle)?; - let mut zlib = resource.inner.borrow_mut(); - - zlib.do_write(flush)?; - Ok((zlib.err, zlib.strm.avail_out, zlib.strm.avail_in)) - }) -} - #[allow(clippy::too_many_arguments)] #[op2(fast)] #[smi] pub fn op_zlib_write( - state: &mut OpState, - #[smi] handle: u32, + #[cppgc] resource: &Zlib, #[smi] flush: i32, #[buffer] input: &[u8], #[smi] in_off: u32, @@ -330,9 +293,11 @@ pub fn op_zlib_write( #[smi] out_len: u32, #[buffer] result: &mut [u32], ) -> Result { - let resource = zlib(state, handle)?; - let mut zlib = resource.inner.borrow_mut(); + let mut zlib = zlib + .as_mut() + .ok_or_else(|| type_error("zlib not initialized"))?; + let flush = Flush::try_from(flush)?; zlib.start_write(input, in_off, in_len, out, out_off, out_len, flush)?; zlib.do_write(flush)?; @@ -346,16 +311,17 @@ pub fn op_zlib_write( #[op2(fast)] #[smi] pub fn op_zlib_init( - state: &mut OpState, - #[smi] handle: u32, + #[cppgc] resource: &Zlib, #[smi] level: i32, #[smi] window_bits: i32, #[smi] mem_level: i32, #[smi] strategy: i32, #[buffer] dictionary: &[u8], ) -> Result { - let resource = zlib(state, handle)?; let mut zlib = resource.inner.borrow_mut(); + let mut zlib = zlib + .as_mut() + .ok_or_else(|| type_error("zlib not initialized"))?; check((8..=15).contains(&window_bits), "invalid windowBits")?; check((-1..=9).contains(&level), "invalid level")?; @@ -392,13 +358,12 @@ pub fn op_zlib_init( #[op2(fast)] #[smi] -pub fn op_zlib_reset( - state: &mut OpState, - #[smi] handle: u32, -) -> Result { - let resource = zlib(state, handle)?; - +pub fn op_zlib_reset(#[cppgc] resource: &Zlib) -> Result { let mut zlib = resource.inner.borrow_mut(); + let mut zlib = zlib + .as_mut() + .ok_or_else(|| type_error("zlib not initialized"))?; + zlib.reset_stream()?; Ok(zlib.err) @@ -406,18 +371,19 @@ pub fn op_zlib_reset( #[op2(fast)] pub fn op_zlib_close_if_pending( - state: &mut OpState, - #[smi] handle: u32, + #[cppgc] resource: &Zlib, ) -> Result<(), AnyError> { - let resource = zlib(state, handle)?; let pending_close = { let mut zlib = resource.inner.borrow_mut(); + let mut zlib = zlib + .as_mut() + .ok_or_else(|| type_error("zlib not initialized"))?; + zlib.write_in_progress = false; zlib.pending_close }; if pending_close { - drop(resource); - if let Ok(res) = state.resource_table.take_any(handle) { + if let Some(mut res) = resource.inner.borrow_mut().take() { res.close(); } } diff --git a/ext/node/polyfills/_zlib_binding.mjs b/ext/node/polyfills/_zlib_binding.mjs index 729af9b352b5d8..5e08ebdcf6abe8 100644 --- a/ext/node/polyfills/_zlib_binding.mjs +++ b/ext/node/polyfills/_zlib_binding.mjs @@ -50,8 +50,8 @@ import { op_zlib_new, op_zlib_reset, op_zlib_write, - op_zlib_write_async, } from "ext:core/ops"; +import process from "node:process"; const writeResult = new Uint32Array(2); @@ -124,19 +124,18 @@ class Zlib { out_off, out_len, ) { - op_zlib_write_async( - this.#handle, - flush ?? Z_NO_FLUSH, - input, - in_off, - in_len, - out, - out_off, - out_len, - ).then(([err, availOut, availIn]) => { - if (this.#checkError(err)) { - this.callback(availIn, availOut); - } + process.nextTick(() => { + const [availOut, availIn] = this.writeSync( + flush ?? Z_NO_FLUSH, + input, + in_off, + in_len, + out, + out_off, + out_len, + ); + + this.callback(availOut, availIn); }); return this; diff --git a/tests/unit_node/zlib_test.ts b/tests/unit_node/zlib_test.ts index 9122997fed7195..c7e9bbca9ba17f 100644 --- a/tests/unit_node/zlib_test.ts +++ b/tests/unit_node/zlib_test.ts @@ -36,7 +36,7 @@ Deno.test("brotli compression async", async () => { assertEquals(decompressed.toString(), "hello world"); }); -Deno.test("gzip compression sync", { sanitizeResources: false }, () => { +Deno.test("gzip compression sync", () => { const buf = Buffer.from("hello world"); const compressed = gzipSync(buf); const decompressed = unzipSync(compressed); @@ -94,7 +94,6 @@ Deno.test("brotli end-to-end with 4097 bytes", () => { Deno.test( "zlib create deflate with dictionary", - { sanitizeResources: false }, async () => { const { promise, resolve } = Promise.withResolvers(); const handle = createDeflate({ @@ -111,8 +110,6 @@ Deno.test( Deno.test( "zlib flush i32", - // FIXME: Handle is not closed properly - { sanitizeResources: false }, function () { const handle = createDeflate({ // @ts-expect-error: passing non-int flush value @@ -142,7 +139,6 @@ Deno.test("should work with a buffer from an encoded string", () => { Deno.test( "zlib compression with dataview", - { sanitizeResources: false }, () => { const buf = Buffer.from("hello world"); const compressed = gzipSync(new DataView(buf.buffer)); @@ -151,9 +147,7 @@ Deno.test( }, ); -Deno.test("zlib compression with an encoded string", { - sanitizeResources: false, -}, () => { +Deno.test("zlib compression with an encoded string", () => { const encoder = new TextEncoder(); const buffer = encoder.encode("hello world"); const compressed = gzipSync(buffer); From c8ecfc033153f9163ae00e04c418b0dd293b4712 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 19 Jun 2024 20:14:44 +0530 Subject: [PATCH 2/3] lint --- ext/node/ops/zlib/mod.rs | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/ext/node/ops/zlib/mod.rs b/ext/node/ops/zlib/mod.rs index a17eb30b2711b6..7f2871a16ce3ea 100644 --- a/ext/node/ops/zlib/mod.rs +++ b/ext/node/ops/zlib/mod.rs @@ -1,13 +1,9 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -use deno_core::error::bad_resource_id; use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::op2; -use deno_core::OpState; use std::borrow::Cow; use std::cell::RefCell; -use std::future::Future; -use std::rc::Rc; use zlib::*; mod alloc; @@ -29,14 +25,6 @@ fn check(condition: bool, msg: &str) -> Result<(), AnyError> { } } -#[inline] -fn zlib(state: &mut OpState, handle: u32) -> Result, AnyError> { - state - .resource_table - .get::(handle) - .map_err(|_| bad_resource_id()) -} - #[derive(Default)] struct ZlibInner { dictionary: Option>, @@ -269,7 +257,7 @@ pub fn op_zlib_new(#[smi] mode: i32) -> Result { #[op2(fast)] pub fn op_zlib_close(#[cppgc] resource: &Zlib) -> Result<(), AnyError> { let mut resource = resource.inner.borrow_mut(); - let mut zlib = resource + let zlib = resource .as_mut() .ok_or_else(|| type_error("zlib not initialized"))?; @@ -294,7 +282,7 @@ pub fn op_zlib_write( #[buffer] result: &mut [u32], ) -> Result { let mut zlib = resource.inner.borrow_mut(); - let mut zlib = zlib + let zlib = zlib .as_mut() .ok_or_else(|| type_error("zlib not initialized"))?; @@ -319,7 +307,7 @@ pub fn op_zlib_init( #[buffer] dictionary: &[u8], ) -> Result { let mut zlib = resource.inner.borrow_mut(); - let mut zlib = zlib + let zlib = zlib .as_mut() .ok_or_else(|| type_error("zlib not initialized"))?; @@ -360,7 +348,7 @@ pub fn op_zlib_init( #[smi] pub fn op_zlib_reset(#[cppgc] resource: &Zlib) -> Result { let mut zlib = resource.inner.borrow_mut(); - let mut zlib = zlib + let zlib = zlib .as_mut() .ok_or_else(|| type_error("zlib not initialized"))?; @@ -375,7 +363,7 @@ pub fn op_zlib_close_if_pending( ) -> Result<(), AnyError> { let pending_close = { let mut zlib = resource.inner.borrow_mut(); - let mut zlib = zlib + let zlib = zlib .as_mut() .ok_or_else(|| type_error("zlib not initialized"))?; @@ -384,7 +372,7 @@ pub fn op_zlib_close_if_pending( }; if pending_close { if let Some(mut res) = resource.inner.borrow_mut().take() { - res.close(); + let _ = res.close(); } } From 0634310843ded1a2a0eb5dfb3fa66f0e303be77c Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 19 Jun 2024 20:26:52 +0530 Subject: [PATCH 3/3] fix --- ext/node/polyfills/_zlib_binding.mjs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ext/node/polyfills/_zlib_binding.mjs b/ext/node/polyfills/_zlib_binding.mjs index 5e08ebdcf6abe8..118f9edc47a1c9 100644 --- a/ext/node/polyfills/_zlib_binding.mjs +++ b/ext/node/polyfills/_zlib_binding.mjs @@ -125,7 +125,7 @@ class Zlib { out_len, ) { process.nextTick(() => { - const [availOut, availIn] = this.writeSync( + const res = this.writeSync( flush ?? Z_NO_FLUSH, input, in_off, @@ -135,7 +135,10 @@ class Zlib { out_len, ); - this.callback(availOut, availIn); + if (res) { + const [availOut, availIn] = res; + this.callback(availOut, availIn); + } }); return this;