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.

Sign up for GitHub

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

fix(ext/node): node compatibility issue missing fd in createServer callback socket object #27789

Merged
merged 9 commits into from
Feb 26, 2025
Merged

fix(ext/node): node compatibility issue missing fd in createServer callback socket object #27789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2b5a7c6
fix(ext/net): node compatibility issue missing fd in createServer cal…
muthu90tech Jan 23, 2025
35c8ddd
use internal symbol for fd
muthu90tech Jan 25, 2025
0df30bd
use internal symbol to read fd
muthu90tech Jan 25, 2025
0899db8
fix proto literal
muthu90tech Jan 25, 2025
9ef9d4e
simplify assignment
muthu90tech Feb 17, 2025
8e66d6d
unblock windows build issue
muthu90tech Feb 20, 2025
42d1cdc
address linux lint issue
muthu90tech Feb 20, 2025
5c25b55
import in macro block
muthu90tech Feb 20, 2025
501302a
revert lib.deno_net.d.ts
muthu90tech Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions ext/net/01_net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const {
BadResourcePrototype,
InterruptedPrototype,
internalRidSymbol,
internalFdSymbol,
createCancelHandle,
} = core;
import {
Expand Down Expand Up @@ -99,13 +100,17 @@ class Conn {

#readable;
#writable;

constructor(rid, remoteAddr, localAddr) {
constructor(rid, remoteAddr, localAddr, fd) {
ObjectDefineProperty(this, internalRidSymbol, {
__proto__: null,
enumerable: false,
value: rid,
});
ObjectDefineProperty(this, internalFdSymbol, {
__proto__: null,
enumerable: false,
value: fd,
});
this.#rid = rid;
this.#remoteAddr = remoteAddr;
this.#localAddr = localAddr;
Expand Down Expand Up @@ -211,8 +216,8 @@ class UpgradedConn extends Conn {
class TcpConn extends Conn {
#rid = 0;

constructor(rid, remoteAddr, localAddr) {
super(rid, remoteAddr, localAddr);
constructor(rid, remoteAddr, localAddr, fd) {
super(rid, remoteAddr, localAddr, fd);
ObjectDefineProperty(this, internalRidSymbol, {
__proto__: null,
enumerable: false,
Expand Down Expand Up @@ -278,12 +283,12 @@ class Listener {
}
this.#promise = promise;
if (this.#unref) core.unrefOpPromise(promise);
const { 0: rid, 1: localAddr, 2: remoteAddr } = await promise;
const { 0: rid, 1: localAddr, 2: remoteAddr, 3: fd } = await promise;
this.#promise = null;
if (this.addr.transport == "tcp") {
localAddr.transport = "tcp";
remoteAddr.transport = "tcp";
return new TcpConn(rid, remoteAddr, localAddr);
return new TcpConn(rid, remoteAddr, localAddr, fd);
} else if (this.addr.transport == "unix") {
return new UnixConn(
rid,
Expand Down
19 changes: 17 additions & 2 deletions ext/net/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ use crate::resolve_addr::resolve_addr_sync;
use crate::tcp::TcpListener;
use crate::NetPermissions;

pub type Fd = u32;

#[derive(Serialize, Clone, Debug)]
#[serde(rename_all = "camelCase")]
pub struct TlsHandshakeInfo {
Expand Down Expand Up @@ -165,7 +167,7 @@ pub(crate) fn accept_err(e: std::io::Error) -> NetError {
pub async fn op_net_accept_tcp(
state: Rc<RefCell<OpState>>,
#[smi] rid: ResourceId,
) -> Result<(ResourceId, IpAddr, IpAddr), NetError> {
) -> Result<(ResourceId, IpAddr, IpAddr, Option<Fd>), NetError> {
let resource = state
.borrow()
.resource_table
Expand All @@ -180,14 +182,27 @@ pub async fn op_net_accept_tcp(
.try_or_cancel(cancel)
.await
.map_err(accept_err)?;
let mut _fd_raw: Option<Fd> = None;
Copy link
Member

Choose a reason for hiding this comment

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

nit: probably this shouldn't be prefixed with _ as the value is used later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kt3k , yes I initially had fd_raw, but the lint on linux complained about

error: value assigned to `fd_raw` is never read
   --> ext/net/ops.rs:185:11
    |
185 |   let mut fd_raw: Option<Fd> = None;
    |           ^^^^^^
    |
    = help: maybe it is overwritten before being read?
    = note: `-D unused-assignments` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unused_assignments)]`

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Makes sense

#[cfg(not(windows))]
{
use std::os::fd::AsFd;
use std::os::fd::AsRawFd;
let fd = tcp_stream.as_fd();
_fd_raw = Some(fd.as_raw_fd() as u32);
}
let local_addr = tcp_stream.local_addr()?;
let remote_addr = tcp_stream.peer_addr()?;

let mut state = state.borrow_mut();
let rid = state
.resource_table
.add(TcpStreamResource::new(tcp_stream.into_split()));
Ok((rid, IpAddr::from(local_addr), IpAddr::from(remote_addr)))
Ok((
rid,
IpAddr::from(local_addr),
IpAddr::from(remote_addr),
_fd_raw,
))
}

#[op2(async)]
Expand Down
8 changes: 6 additions & 2 deletions ext/node/polyfills/internal_binding/tcp_wrap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
// TODO(petamoriken): enable prefer-primordials for node polyfills
// deno-lint-ignore-file prefer-primordials

import { core } from "ext:core/mod.js";
const { internalFdSymbol } = core;
import { notImplemented } from "ext:deno_node/_utils.ts";
import { unreachable } from "ext:deno_node/_util/asserts.ts";
import { ConnectionWrap } from "ext:deno_node/internal_binding/connection_wrap.ts";
Expand Down Expand Up @@ -141,6 +143,10 @@ export class TCP extends ConnectionWrap {
}
}

get fd() {
return this[kStreamBaseField]?.[internalFdSymbol];
}

/**
* Opens a file descriptor.
* @param fd The file descriptor to open.
Expand Down Expand Up @@ -221,7 +227,6 @@ export class TCP extends ConnectionWrap {
const address = listener.addr as Deno.NetAddr;
this.#address = address.hostname;
this.#port = address.port;

this.#listener = listener;

// TODO(kt3k): Delays the accept() call 2 ticks. Deno.Listener can't be closed
Expand Down Expand Up @@ -458,7 +463,6 @@ export class TCP extends ConnectionWrap {

// Reset the backoff delay upon successful accept.
this.#acceptBackoffDelay = undefined;

const connectionHandle = new TCP(socketType.SOCKET, connection);
this.#connections++;

Expand Down