BREAKING(ext/net): improved error code accuracy (#25383)

Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>

ext/net/ops.rs

@@ -69,7 +69,6 @@ impl From<SocketAddr> for IpAddr {
 }
 
 pub(crate) fn accept_err(e: std::io::Error) -> AnyError {
-  // FIXME(bartlomieju): compatibility with current JS implementation
   if let std::io::ErrorKind::Interrupted = e.kind() {
     bad_resource("Listener has been closed")
   } else {

ext/net/ops_tls.rs

@@ -298,10 +298,10 @@ where
     .resource_table
     .take::<TcpStreamResource>(rid)?;
   // This TCP connection might be used somewhere else. If it's the case, we cannot proceed with the
-  // process of starting a TLS connection on top of this TCP connection, so we just return a bad
+  // process of starting a TLS connection on top of this TCP connection, so we just return a Busy error.
-  // resource error. See also: https://github.com/denoland/deno/pull/16242
+  // See also: https://github.com/denoland/deno/pull/16242
   let resource = Rc::try_unwrap(resource_rc)
-    .map_err(|_| bad_resource("TCP stream is currently in use"))?;
+    .map_err(|_| custom_error("Busy", "TCP stream is currently in use"))?;
   let (read_half, write_half) = resource.into_inner();
   let tcp_stream = read_half.reunite(write_half)?;
 
@@ -526,7 +526,6 @@ pub async fn op_net_accept_tls(
     match listener.accept().try_or_cancel(&cancel_handle).await {
       Ok(tuple) => tuple,
       Err(err) if err.kind() == ErrorKind::Interrupted => {
-        // FIXME(bartlomieju): compatibility with current JS implementation.
         return Err(bad_resource("Listener has been closed"));
       }
       Err(err) => return Err(err.into()),

ext/net/raw.rs

@@ -1,8 +1,8 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 use crate::io::TcpStreamResource;
 use crate::ops_tls::TlsStreamResource;
-use deno_core::error::bad_resource;
 use deno_core::error::bad_resource_id;
+use deno_core::error::custom_error;
 use deno_core::error::AnyError;
 use deno_core::AsyncRefCell;
 use deno_core::CancelHandle;
@@ -70,7 +70,7 @@ impl<T: NetworkStreamListenerTrait + 'static> NetworkListenerResource<T> {
   ) -> Result<Option<NetworkStreamListener>, AnyError> {
     if let Ok(resource_rc) = resource_table.take::<Self>(listener_rid) {
       let resource = Rc::try_unwrap(resource_rc)
-        .map_err(|_| bad_resource("Listener is currently in use"))?;
+        .map_err(|_| custom_error("Busy", "Listener is currently in use"))?;
       return Ok(Some(resource.listener.into_inner().into()));
     }
     Ok(None)
@@ -334,7 +334,7 @@ pub fn take_network_stream_resource(
   {
     // This TCP connection might be used somewhere else.
     let resource = Rc::try_unwrap(resource_rc)
-      .map_err(|_| bad_resource("TCP stream is currently in use"))?;
+      .map_err(|_| custom_error("Busy", "TCP stream is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let tcp_stream = read_half.reunite(write_half)?;
     return Ok(NetworkStream::Tcp(tcp_stream));
@@ -344,7 +344,7 @@ pub fn take_network_stream_resource(
   {
     // This TLS connection might be used somewhere else.
     let resource = Rc::try_unwrap(resource_rc)
-      .map_err(|_| bad_resource("TLS stream is currently in use"))?;
+      .map_err(|_| custom_error("Busy", "TLS stream is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let tls_stream = read_half.unsplit(write_half);
     return Ok(NetworkStream::Tls(tls_stream));
@@ -356,7 +356,7 @@ pub fn take_network_stream_resource(
   {
     // This UNIX socket might be used somewhere else.
     let resource = Rc::try_unwrap(resource_rc)
-      .map_err(|_| bad_resource("UNIX stream is currently in use"))?;
+      .map_err(|_| custom_error("Busy", "Unix socket is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let unix_stream = read_half.reunite(write_half)?;
     return Ok(NetworkStream::Unix(unix_stream));

runtime/ops/http.rs

@@ -2,8 +2,8 @@
 
 use std::rc::Rc;
 
-use deno_core::error::bad_resource;
 use deno_core::error::bad_resource_id;
+use deno_core::error::custom_error;
 use deno_core::error::AnyError;
 use deno_core::op2;
 use deno_core::OpState;
@@ -27,10 +27,10 @@ fn op_http_start(
     .take::<TcpStreamResource>(tcp_stream_rid)
   {
     // This TCP connection might be used somewhere else. If it's the case, we cannot proceed with the
-    // process of starting a HTTP server on top of this TCP connection, so we just return a bad
+    // process of starting a HTTP server on top of this TCP connection, so we just return a Busy error.
-    // resource error. See also: https://github.com/denoland/deno/pull/16242
+    // See also: https://github.com/denoland/deno/pull/16242
     let resource = Rc::try_unwrap(resource_rc)
-      .map_err(|_| bad_resource("TCP stream is currently in use"))?;
+      .map_err(|_| custom_error("Busy", "TCP stream is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let tcp_stream = read_half.reunite(write_half)?;
     let addr = tcp_stream.local_addr()?;
@@ -42,10 +42,10 @@ fn op_http_start(
     .take::<TlsStreamResource>(tcp_stream_rid)
   {
     // This TLS connection might be used somewhere else. If it's the case, we cannot proceed with the
-    // process of starting a HTTP server on top of this TLS connection, so we just return a bad
+    // process of starting a HTTP server on top of this TLS connection, so we just return a Busy error.
-    // resource error. See also: https://github.com/denoland/deno/pull/16242
+    // See also: https://github.com/denoland/deno/pull/16242
     let resource = Rc::try_unwrap(resource_rc)
-      .map_err(|_| bad_resource("TLS stream is currently in use"))?;
+      .map_err(|_| custom_error("Busy", "TLS stream is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let tls_stream = read_half.unsplit(write_half);
     let addr = tls_stream.local_addr()?;
@@ -58,10 +58,10 @@ fn op_http_start(
     .take::<deno_net::io::UnixStreamResource>(tcp_stream_rid)
   {
     // This UNIX socket might be used somewhere else. If it's the case, we cannot proceed with the
-    // process of starting a HTTP server on top of this UNIX socket, so we just return a bad
+    // process of starting a HTTP server on top of this UNIX socket, so we just return a Busy error.
-    // resource error. See also: https://github.com/denoland/deno/pull/16242
+    // See also: https://github.com/denoland/deno/pull/16242
     let resource = Rc::try_unwrap(resource_rc)
-      .map_err(|_| bad_resource("UNIX stream is currently in use"))?;
+      .map_err(|_| custom_error("Busy", "Unix socket is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let unix_stream = read_half.reunite(write_half)?;
     let addr = unix_stream.local_addr()?;

tests/integration/js_unit_tests.rs

@@ -143,8 +143,19 @@ fn js_unit_test(test: String) {
     deno = deno.arg("--unstable-worker-options");
   }
 
-  // TODO(mmastrac): it would be better to just load a test CA for all tests
+  // Some tests require the root CA cert file to be loaded.
-  if test == "websocket_test" || test == "tls_sni_test" {
+  if test == "websocket_test" {
+    deno = deno.arg(format!(
+      "--cert={}",
+      util::testdata_path()
+        .join("tls")
+        .join("RootCA.pem")
+        .to_string_lossy()
+    ));
+  };
+
+  if test == "tls_sni_test" {
+    // TODO(lucacasonato): fix the SNI in the certs so that this is not needed
     deno = deno.arg("--unsafely-ignore-certificate-errors");
   }
 

tests/integration/node_unit_tests.rs

@@ -113,15 +113,18 @@ fn node_unit_test(test: String) {
     .arg("--no-lock")
     .arg("--unstable-broadcast-channel")
     .arg("--unstable-net")
-    // TODO(kt3k): This option is required to pass tls_test.ts,
-    // but this shouldn't be necessary. tls.connect currently doesn't
-    // pass hostname option correctly and it causes cert errors.
-    .arg("--unsafely-ignore-certificate-errors")
     .arg("-A");
+
+  // Some tests require the root CA cert file to be loaded.
+  if test == "http2_test" || test == "http_test" {
+    deno = deno.arg("--cert=./tests/testdata/tls/RootCA.pem");
+  }
+
   // Parallel tests for crypto
   if test.starts_with("crypto/") {
     deno = deno.arg("--parallel");
   }
+
   let mut deno = deno
     .arg(
       util::tests_path()

tests/unit/http_test.ts

@@ -2305,7 +2305,7 @@ Deno.test(
 
     const buf = new Uint8Array(128);
     const readPromise = serverConn.read(buf);
-    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.BadResource);
+    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.Busy);
 
     clientConn.close();
     listener.close();
@@ -2338,7 +2338,7 @@ Deno.test(
 
     const buf = new Uint8Array(128);
     const readPromise = serverConn.read(buf);
-    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.BadResource);
+    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.Busy);
 
     clientConn.close();
     listener.close();
@@ -2362,7 +2362,7 @@ Deno.test(
 
     const buf = new Uint8Array(128);
     const readPromise = serverConn.read(buf);
-    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.BadResource);
+    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.Busy);
 
     clientConn.close();
     listener.close();

tests/unit/net_test.ts

@@ -126,8 +126,6 @@ Deno.test(
     const listener = Deno.listen({ port: listenPort });
     const p = listener.accept();
     listener.close();
-    // TODO(piscisaureus): the error type should be `Interrupted` here, which
-    // gets thrown, but then ext/net catches it and rethrows `BadResource`.
     await assertRejects(
       () => p,
       Deno.errors.BadResource,
@@ -168,7 +166,7 @@ Deno.test(
       } else if (e.message === "Another accept task is ongoing") {
         acceptErrCount++;
       } else {
-        throw new Error("Unexpected error message");
+        throw e;
       }
     };
     const p = listener.accept().catch(checkErr);

tests/unit/tls_test.ts

@@ -81,7 +81,7 @@ Deno.test(
     // `Deno.startTls` cannot consume the connection.
     await assertRejects(
       () => Deno.startTls(clientConn, { hostname }),
-      Deno.errors.BadResource,
+      Deno.errors.Busy,
     );
 
     serverConn.close();

tests/unit_node/http2_test.ts

@@ -10,7 +10,7 @@ import * as net from "node:net";
 import { assert, assertEquals } from "@std/assert";
 import { curlRequest } from "../unit/test_util.ts";
 
-for (const url of ["http://127.0.0.1:4246", "https://127.0.0.1:4247"]) {
+for (const url of ["http://localhost:4246", "https://localhost:4247"]) {
   Deno.test(`[node/http2 client] ${url}`, {
     ignore: Deno.build.os === "windows",
   }, async () => {
@@ -155,7 +155,7 @@ Deno.test("[node/http2.createServer()]", {
     res.end();
   });
   server.listen(0);
-  const port = (<net.AddressInfo> server.address()).port;
+  const port = (server.address() as net.AddressInfo).port;
   const endpoint = `http://localhost:${port}`;
 
   const response = await curlRequest([

tests/unit_node/tls_test.ts

@@ -149,10 +149,12 @@ Deno.test("tls.createServer creates a TLS server", async () => {
     },
   );
   server.listen(0, async () => {
-    const conn = await Deno.connectTls({
+    const tcpConn = await Deno.connect({
-      hostname: "127.0.0.1",
       // deno-lint-ignore no-explicit-any
       port: (server.address() as any).port,
+    });
+    const conn = await Deno.startTls(tcpConn, {
+      hostname: "localhost",
       caCerts: [rootCaCert],
     });