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.

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

Add fifo as a transport option #1351

Open
karthiknadig opened this issue Nov 3, 2023 · 7 comments
Open

Add fifo as a transport option #1351

karthiknadig opened this issue Nov 3, 2023 · 7 comments
Labels
feature-request Request for new features or functionality
Milestone

Comments

@karthiknadig
Copy link
Member

This code should use fs.mkfifo on non-windows:

let connectResolve: (value: [MessageReader, MessageWriter]) => void;
const connected = new Promise<[MessageReader, MessageWriter]>((resolve, _reject) => {
connectResolve = resolve;
});
return new Promise<PipeTransport>((resolve, reject) => {
let server: Server = createServer((socket: Socket) => {
server.close();
connectResolve([
new SocketMessageReader(socket, encoding),
new SocketMessageWriter(socket, encoding)
]);
});
server.on('error', reject);
server.listen(pipeName, () => {
server.removeListener('error', reject);
resolve({
onConnected: () => { return connected; }
});
});
});

@karthiknadig
Copy link
Member Author

Unix Domain Socket (UDS) causes a problem in python for adaption of pip transport in LSP/DAP/testing where we need to communicate back to node process and we use VSCode JSON RPC package. When I say LSP/DAP/testing, we have plans to migrate all socket communication to use namedpipes. There are packages in python that explicitly disable sockets on load, and we often get bitten by firewall rules. Stdio communication also gets impacted by packages trying to write to stdio on load. Sometimes this occurs even before any of our packages are loaded for us to capture the stdio file descriptors. Another way this is a problem is when, pythons logging is enabled to write logs to stdout. We have several reasons to get off of stdio, and sockets is not a good option, so pipes are the next best thing.

How we discovered this problem: in python named pipes can be connected to using open, like with open("<pipe-name", "r+b") as pipe: gives you a stream object that read/write to pipes. This should work on all os'es. It worked on windows, when I tested this on linux, open would come back with file does not exist. I could connect to the node "pipe" using netcat or by using python sockets by setting the socket family to (AF_UNIX). Both of which means that it is using Unix Domain Socket.

The recommended change is on non-windows to use mkfifo to create the namedpipe (or fifo), and provide a similar wrapper to expose the read write streams.

@dbaeumer
Copy link
Member

dbaeumer commented Nov 6, 2023

@karthiknadig could you please point me to the fs.mkfifo in NodeJS. I was not able to find it. All I could find is stat related API to detect if a handle is a fifo.

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Nov 6, 2023
@karthiknadig
Copy link
Member Author

karthiknadig commented Nov 6, 2023

@dbaeumer I will share code sample. You basically spawn mkfifo as a subprocess to create it. (https://www.gnu.org/software/coreutils/mkfifo)

@karthiknadig
Copy link
Member Author

Here is an example:

   let connectResolve: (value: [rpc.MessageReader, rpc.MessageWriter]) => void;

    const connected = new Promise<[rpc.MessageReader, rpc.MessageWriter]>((resolve, _reject) => {
        connectResolve = resolve;
    });
    return new Promise((resolve, reject) => {
        const proc = child_process.spawn('mkfifo', ['-m', '0666', pipeName]);
        proc.on('error', reject);
        proc.on('exit', (code, signal) => {
            if (code !== 0) {
                reject(new Error(`Failed to create fifo pipe ${pipeName}: ${signal}`));
            }
            fs.open(pipeName, constants.O_RDWR, (err, fd) => {
                if (err) {
                    reject(err);
                }
                const socket = new net.Socket({ fd, readable: true, writable: true });
                connectResolve([
                    new rpc.SocketMessageReader(socket, 'utf-8'),
                    new rpc.SocketMessageWriter(socket, 'utf-8'),
                ]);
            });
            resolve({ onConnected: () => connected });
        });
    });

You can also replace net.Socket with just plain stream from fs.createReadStream and fs.createWriteStream.

@dbaeumer
Copy link
Member

dbaeumer commented Nov 7, 2023

@karthiknadig thanks for the code snippet. Any desire to provide a PR for this :-) This code should go somewhere here:

let connectResolve: (value: [MessageReader, MessageWriter]) => void;
and
const socket: Socket = createConnection(pipeName);

I still think that we need to add a new transport kind fifo for this. Otherwise we might break things if users upgrade the client but not the server.

@dbaeumer dbaeumer added feature-request Request for new features or functionality and removed info-needed Issue requires more information from poster labels Nov 7, 2023
@dbaeumer dbaeumer added this to the 3.18 milestone Nov 7, 2023
@karthiknadig
Copy link
Member Author

@dbaeumer I investigated more into how fifo are done in various linux variants and mac. There is lot of variance in how fifo are handled in each OS. The main issues comes down to read-write ordering. The requirement is that the read side should open the fifo in read mode, before the write side opens it. LS Client should open the fifo in read mode and wait for LS Server to open the fifo in read mode on its side before both LS Client and LS Server open write mode streams. This coordination is going to be difficult, as we don't really have a protocol way to say server has connected. fifos don't have a way to detect this, we will need to do this manually. The behavior of this ordering varies between flavors on linux. The fact that there is an ordering requirement is a blocker to me.

So, I don't think we can switch the communication over from stdio for python scenarios unfortunately.

@dbaeumer
Copy link
Member

dbaeumer commented Nov 9, 2023

@karthiknadig thanks for the investigation. One idea for working around the ordering is to have some ready/tag files in the file system to synchronize this.

@karthiknadig karthiknadig changed the title Pipe transport on unix should use fifo not Unix Domain Socket Add fifo as a transport option Nov 17, 2023
@dbaeumer dbaeumer modified the milestones: 3.18, Backlog Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

2 participants