fix(ext/node): use cppgc for node:zlib (denoland#24267)

sbmsr · Jun 20, 2024 · 0b65d02 · 0b65d02
1 parent 2cfaee0
commit 0b65d02
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 101 deletions.
diff --git 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
@@ -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<Rc<Zlib>, AnyError> {
- state
- .resource_table
- .get::<Zlib>(handle)
- .map_err(|_| bad_resource_id())
-}
-
 #[derive(Default)]
 struct ZlibInner {
  dictionary: Option<Vec<u8>>,
@@ -242,7 230,7 @@ impl ZlibInner {
 }
 
 struct Zlib {
- inner: RefCell<ZlibInner>,
  inner: RefCell<Option<ZlibInner>>,
 }
 
 impl deno_core::Resource for Zlib {
@@ -251,76 239,39 @@ impl deno_core::Resource for Zlib {
  }
 }
 
-#[op2(fast)]
-#[smi]
-pub fn op_zlib_new(
- state: &mut OpState,
- #[smi] mode: i32,
-) -> Result<u32, AnyError> {
 #[op2]
 #[cppgc]
 pub fn op_zlib_new(#[smi] mode: i32) -> Result<Zlib, AnyError> {
  let mode = Mode::try_from(mode)?;
 
  let inner = ZlibInner {
  mode,
  ..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 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()?;
 
  Ok(())
 }
 
-#[allow(clippy::too_many_arguments)]
-#[op2(async)]
-#[serde]
-pub fn op_zlib_write_async(
- state: Rc<RefCell<OpState>>,
- #[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<impl Future<Output = Result<(i32, u32, u32), AnyError>>, 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 281,11 @@ pub fn op_zlib_write(
  #[smi] out_len: u32,
  #[buffer] result: &mut [u32],
 ) -> Result<i32, AnyError> {
- let resource = zlib(state, handle)?;
-
  let mut zlib = resource.inner.borrow_mut();
  let 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 299,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<i32, AnyError> {
- let resource = zlib(state, handle)?;
  let mut zlib = resource.inner.borrow_mut();
  let 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,33 346,33 @@ pub fn op_zlib_init(
 
 #[op2(fast)]
 #[smi]
-pub fn op_zlib_reset(
- state: &mut OpState,
- #[smi] handle: u32,
-) -> Result<i32, AnyError> {
- let resource = zlib(state, handle)?;
-
 pub fn op_zlib_reset(#[cppgc] resource: &Zlib) -> Result<i32, AnyError> {
  let mut zlib = resource.inner.borrow_mut();
  let zlib = zlib
  .as_mut()
  .ok_or_else(|| type_error("zlib not initialized"))?;
 
  zlib.reset_stream()?;
 
  Ok(zlib.err)
 }
 
 #[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 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) {
- res.close();
  if let Some(mut res) = resource.inner.borrow_mut().take() {
  let _ = res.close();
  }
  }
 

diff --git 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,18 124,20 @@ 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 res = this.writeSync(
  flush ?? Z_NO_FLUSH,
  input,
  in_off,
  in_len,
  out,
  out_off,
  out_len,
  );
 
  if (res) {
  const [availOut, availIn] = res;
  this.callback(availOut, availIn);
  }
  });
 

diff --git 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<void>();
  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);