MatrixAI / js-mdns

Multicast DNS Stack for TypeScript/JavaScript Applications
https://matrixai.github.io/js-mdns/
Apache License 2.0
0 stars 0 forks source link

`Socket.prototype._handle is deprecated` warning is caused by `MDNS.ts` #42

Open CMCDragonkai opened 3 months ago

CMCDragonkai commented 3 months ago

Describe the bug

Inside MDNS.ts, it's using _handle in a few places. In particular:

        if (platform === 'linux' && disableLinuxMulticastAllOption) {
          socketUtils.disableSocketMulticastAll(
            (unicastSocket as any)._handle.fd,
          );
        }
          if (platform === 'linux' && disableLinuxMulticastAllOption) {
            socketUtils.disableSocketMulticastAll((socket as any)._handle.fd);
          }

The usage of _handle in the Socket prototype causes warnings.

It looks like this in PK CLI:

(node:500471) [DEP0112] DeprecationWarning: Socket.prototype._handle is deprecated
(Use `node --trace-deprecation ...` to show where the warning was created)

Right after starting the agent.

Find an alternative way to do the above, or we need some way of disabling the warning? This is some sort of internal property right?

Apparently Node allows disabling warnings on startup.

To Reproduce

  1. Run Polykey-CLI polykey agent start
  2. Observe warning in the terminal

Expected behavior

No warnings should occur.

Additional context

Notify maintainers

@amydevs

linear[bot] commented 3 months ago

ENG-394 `Socket.prototype._handle` is deprecated` warning is caused by `MDNS.ts`

CMCDragonkai commented 3 months ago

One way to get around this warning is potentially doing all the work in the C++ level? One way to do this is pass the reference to the JS socket object to the native code side, and get the native code to do the necessary referencing of the underlying FD.

Also can we start changing all our native code to Rust to align with js-quic and js-exec? I know js-db is still pending, but it's usually better.

Look at node native code docs as to how to directly interact with socket objects.

aryanjassal commented 2 weeks ago

If we are only using the net.Socket._handle to extract the fd of the socket, can't we just create the socket with a predetermined fd and reference that wherever we need to?

The new net.Socket() constructor has an option to specify the fd. We can use the C++ socketUtils to find the closest unoccupied file descriptor, and use that while construction, and keep track of that in a Map or a Record. We can then later reference that when we need to call disableSocketMulticastAll.

Doing it this way would be simpler than directly interacting with the actual Socket object.

https://nodejs.org/api/net.html#new-netsocketoptions

@cmcdragonkai

CMCDragonkai commented 2 weeks ago

I'm not really sure what that means. You'd have to try it. But my understanding is that we are creating sockets for this purpose and so using the rust native code is better anyway to align all of our native libraries.

aryanjassal commented 2 weeks ago

After looking over the source code a bit more, my solution feels more like a temporary patch than a permanent solution. As I couldn't find much information about this online, I asked ChatGPT, and it agreed with you that creating a native binding which manages the entire socket lifetime seems to be the best solution, as no API currently exists which allows us to do the fine-grained multicast manipulation or get the fd without relying on _handle.

amydevs commented 3 days ago

@aryanjassal i've tried to implement the solution you had suggested originally when working on it in the https://github.com/MatrixAI/js-mdns/tree/feature-remove-warning branch. However, I was not able to successfully get the node socket to correctly attach to the fd of the existing socket. There should be a test written in that repo that demonstrates this behaviour.

tegefaulkes commented 3 days ago

If I recall, the socket modification was to avoid some extra logic in the js-mdns library to handle it. We may just have to do that in the end to resolve this. @amy I don't recall the exact details of this. Can you give a summary of why the socket was modified leading to this warning and what it was trying to avoid in the code. So basically how need to fix this.