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

CloseWrite() is non-portable and may leave client connection hanging #257

Open
nicks opened this issue Aug 22, 2022 · 1 comment
Open

CloseWrite() is non-portable and may leave client connection hanging #257

nicks opened this issue Aug 22, 2022 · 1 comment

Comments

@nicks
Copy link

nicks commented Aug 22, 2022

We currently use winio's named pipe implementation to create http duplex sockets.

We use CloseWrite() to close the server side of the socket.

This works ok if the client is also using winio. But if the client uses a different named pipe implemenation, CloseWrite() does not behave correctly. (in particular, NodeJS/libuv's implementation)

My (very naive) understanding is the winio uses a 0-length message to signal the end of the socket.

// If this was the result of a zero-byte read, then

which other libraries do not support.

Can you all advise on if there's something we should be doing differently here?

Thank you!


Detailed repro steps:

Here's how this breaks things in application-level code: apocas/dockerode#534

Here's a more detailed repro case:

server side:

// main.go

package main

import (
	"log"
	"fmt"
	"github.com/Microsoft/go-winio"
)

type CloseWriter interface {
	CloseWrite() error
}

func main() {
	pipe, err := winio.ListenPipe("//./pipe/test-socket", &winio.PipeConfig{
		MessageMode:        true,
		InputBufferSize:    65536,
		OutputBufferSize:   65536,
	})
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println("Accepting connections on //./pipe/test-socket")
	for {

		conn, err := pipe.Accept()
		if err != nil {
			log.Fatal(err)
	  }
		fmt.Println("opened socket")
  	go func() {

			conn.Write([]byte("HTTP/1.1 101 Web Socket Protocol Handshake\r\n" +
				"Upgrade: WebSocket\r\n" +
				"Connection: Upgrade\r\n" +
				"\r\n"));
			conn.Write([]byte("closing socket\r\n"))
			conn.(CloseWriter).CloseWrite()
			fmt.Println("closed socket")
		}()
	}
}

client side:

// index.js
const http = require('http');
const options = {
  headers: {
    'Connection': 'Upgrade',
    'Upgrade': 'tcp'
  },
  socketPath: '//./pipe/test-socket'
};

const req = http.request(options);
req.on('upgrade', (res, socket, upgradeHead) => {
    console.log('connected to socket');
    socket.allowHalfOpen = false;
    socket.on('data', (data) => {
        console.log('socket received data:', data.toString());
    });
    socket.on('end', () => {
        console.log('socket closed');
    });
});
req.end();

Then run (in separate terminals):

go run ./main.go
node index.js

expected behavior: WinIO should close the socket and nodejs should detect the 'end' event.

actual behavior: nodejs hangs indefinitely

Other notes: The workaround is to use Close() on the Go side instead of CloseWrite(). This closes the whole duplex (rather than closing just one side)

@nicks
Copy link
Author

nicks commented Aug 22, 2022

i also understand that this may end up being a libuv bug -- i couldn't find much documentation on the 0-length message approach to CloseWrite() that winio does, or if this is a generally accepted pattern.

@nicks nicks changed the title CloseWrite() is non-portable CloseWrite() is non-portable and may leave client connection hanging Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant