fix(ext/net): node compatibility issue missing fd in createServer callback socket object #27789
Conversation
…lback socket object This PR will address the node compatibility issue where, the callback socket object in net.createServer is missing a fd in socket._handle fixes: denoland#27788
muthu90tech
force-pushed
the
server_socket_fd
branch
from
0f61f5f to
ffde45b
Compare
ext/net/lib.deno_net.d.ts
Outdated
| @@ -138,6 +138,8 @@ declare namespace Deno { | |||
|
|
|||
| readonly readable: ReadableStream<Uint8Array>; | |||
| readonly writable: WritableStream<Uint8Array>; | |||
|
|
|||
| readonly fd: number; | |||
There was a problem hiding this comment.
I don't think this is desirable addition to Deno API because no other resources expose file descriptor.
We generally use rid as a substitute to fd in node compat layer. Doesn't it work to assign conn.rid to handle.fd for TCP handle for your use case?
There was a problem hiding this comment.
Hi thanks for the review, since the rid is a pointer to the internal tcpstream resource and is an abstraction, we would like to print the actual file descriptor the connection is using for observability purposes. Thank you
There was a problem hiding this comment.
@ry @bartlomieju What do you think?
There was a problem hiding this comment.
I'm fine exposing fd in the Node compat API, but we shouldn't do it in the Deno APIs. IMO this should be handled the same way we're passing resource IDs between Deno APIs and Node APIs (via a private symbol)
There was a problem hiding this comment.
@bartlomieju Got it, I pushed another commit to use internal symbols to expose fd, thank you
denoland/deno_core#1061
please take a look at this PR: denoland/deno#27789 Use an internal symbol to expose fd created after establishing a socket connection in the server side. The node apis expose fd, this ensures the deno node apis are compatible.
…ol in deno_core Please take a look at PR: denoland/deno#27789 This change will help to expose fd to server when a socket connection is established.
|
@muthu90tech I merged your |
This PR will address the node compatibility issue where, the callback socket object in net.createServer is missing a fd in socket._handle
Modified this PR to use internal symbol to expose fd instead of exposing it via lib.deno.d.ts
denoland/deno_core#1061
fixes: #27788