From 3134abefa462ead8bb8e2e4aa8a5b57910f3d430 Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Fri, 27 Sep 2024 16:07:20 +0200 Subject: [PATCH] BREAKING(ext/net): improved error code accuracy (#25383) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bartek Iwańczuk --- ext/net/ops.rs | 1 - ext/net/ops_tls.rs | 7 +++---- ext/net/raw.rs | 10 +++++----- runtime/ops/http.rs | 20 ++++++++++---------- tests/integration/js_unit_tests.rs | 15 +++++++++++++-- tests/integration/node_unit_tests.rs | 11 +++++++---- tests/unit/http_test.ts | 6 +++--- tests/unit/net_test.ts | 4 +--- tests/unit/tls_test.ts | 2 +- tests/unit_node/http2_test.ts | 4 ++-- tests/unit_node/tls_test.ts | 6 ++++-- 11 files changed, 49 insertions(+), 37 deletions(-) diff --git a/ext/net/ops.rs b/ext/net/ops.rs index f2735eda99..5248493f41 100644 --- a/ext/net/ops.rs +++ b/ext/net/ops.rs @@ -69,7 +69,6 @@ impl From 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 { diff --git a/ext/net/ops_tls.rs b/ext/net/ops_tls.rs index 064da5818c..5bc04ceb51 100644 --- a/ext/net/ops_tls.rs +++ b/ext/net/ops_tls.rs @@ -298,10 +298,10 @@ where .resource_table .take::(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 - // resource error. See also: https://github.com/denoland/deno/pull/16242 + // process of starting a TLS connection on top of this TCP connection, so we just return a Busy error. + // 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()), diff --git a/ext/net/raw.rs b/ext/net/raw.rs index f2de760652..a2ebfb5acb 100644 --- a/ext/net/raw.rs +++ b/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 NetworkListenerResource { ) -> Result, AnyError> { if let Ok(resource_rc) = resource_table.take::(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)); diff --git a/runtime/ops/http.rs b/runtime/ops/http.rs index a195a759ee..cec8b0ef8f 100644 --- a/runtime/ops/http.rs +++ b/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::(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 - // resource error. See also: https://github.com/denoland/deno/pull/16242 + // process of starting a HTTP server on top of this TCP connection, so we just return a Busy error. + // 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::(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 - // resource error. See also: https://github.com/denoland/deno/pull/16242 + // process of starting a HTTP server on top of this TLS connection, so we just return a Busy error. + // 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::(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 - // resource error. See also: https://github.com/denoland/deno/pull/16242 + // process of starting a HTTP server on top of this UNIX socket, so we just return a Busy error. + // 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()?; diff --git a/tests/integration/js_unit_tests.rs b/tests/integration/js_unit_tests.rs index 5efb0f2686..577ca043ca 100644 --- a/tests/integration/js_unit_tests.rs +++ b/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 - if test == "websocket_test" || test == "tls_sni_test" { + // Some tests require the root CA cert file to be loaded. + 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"); } diff --git a/tests/integration/node_unit_tests.rs b/tests/integration/node_unit_tests.rs index c8b5b25fb3..d66db5a40e 100644 --- a/tests/integration/node_unit_tests.rs +++ b/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() diff --git a/tests/unit/http_test.ts b/tests/unit/http_test.ts index ea72806d3a..355b155afd 100644 --- a/tests/unit/http_test.ts +++ b/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(); diff --git a/tests/unit/net_test.ts b/tests/unit/net_test.ts index c243da47fa..cfa42b3d36 100644 --- a/tests/unit/net_test.ts +++ b/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); diff --git a/tests/unit/tls_test.ts b/tests/unit/tls_test.ts index aba4d254ce..219f4a4508 100644 --- a/tests/unit/tls_test.ts +++ b/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(); diff --git a/tests/unit_node/http2_test.ts b/tests/unit_node/http2_test.ts index cb939646be..7473a487ad 100644 --- a/tests/unit_node/http2_test.ts +++ b/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 = ( server.address()).port; + const port = (server.address() as net.AddressInfo).port; const endpoint = `http://localhost:${port}`; const response = await curlRequest([ diff --git a/tests/unit_node/tls_test.ts b/tests/unit_node/tls_test.ts index 7daa544c74..93eacd5ec2 100644 --- a/tests/unit_node/tls_test.ts +++ b/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({ - hostname: "127.0.0.1", + const tcpConn = await Deno.connect({ // deno-lint-ignore no-explicit-any port: (server.address() as any).port, + }); + const conn = await Deno.startTls(tcpConn, { + hostname: "localhost", caCerts: [rootCaCert], });