adaptor ResponseWriter - adding Hijack method and pass proper fields

[gilwo]

this change properly add the adaptor ResponseWriter the capability of Hijacker.

and so it will address issues (#946 and #87) , and maybe others I am not aware of

[erikdubbelboer]

Thanks for the addition. One request, can you add a test that shows that this works and makes sure it doesn't break in the future?

[ReneWerner87]

@gilwo friendly ping
test case needed

[erikdubbelboer]

I'm sorry but that's not a good test. A good test would have shown that you need to call ctx.Hijack() for this to really work. The way you implemented it now the net/http handler would think the connection is hijacked but fasthttp would just continue using the connection as it isn't aware of the hijacking.

[erikdubbelboer]

I'm afraid that this won't work. What you implemented now is not how the Hijack function of net/http expects to work. What you need to do is something like this:

type wrapperConn struct {
	wg  sync.WaitGroup
	net.Conn
}

func (c *wrapperConn) Close() error {
	err := c.Conn.Close()
	c.wg.Done()
	return err
}

func (w *netHTTPResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
	conn := wrapperConn{Conn: w.conn}
	conn.wg.Add(1)

	// This Hijack() function should only return when fasthttp calls the Hijack handler, so use a WaitGroup to block until then.
	var wg sync.WaitGroup
	wg.add(1)
	w.ctx.Hijack(func(net.Conn) {
		wg.Done()

		// When this Hijack handler returns, fasthttp will close the connection, so wait until the connection is already closed.
		// The alternative would be to set Server.KeepHijackedConns = true, but we can't control that here.
		conn.wg.Wait()
	})
	wg.Wait()
	
	return conn, bufio.NewReadWriter{bufio.NewReader(conn), bufio.NewWriter(conn)}, nil
}

[gilwo]

Hi, so what the story with this changeset?
I am using 2 different socket.io libraries written in go. both are working fine with net/http and derived frameworks, they do not support fasthttp (via fiber).
I started with the hijack implementation of the adapter to make it work, but after the testing phase and the aforementioned suggestion i noticed the two socket.io framework are not behaving the same on top of fasthttp.

One is taking ownership on the connection after hijacking the request and not reaching the later fasthttp hijack handler - which does not make any difference as the connection ends/closed after socket.io connection is closed.

The other returns from the hijack and continue the flow assuming no one touch (or close) the response/request afterwards (this differ in the flow of the fasthttp handling).

So there it is - i wanted to make the support for both behaviors, this is why the custom hijackhandler.

I am not sure if this pass the review, if not maybe it is reasonable to create another custom adaptor?

[erikdubbelboer]

You mean one returns from the handler and the other doesn't? Both cases should work fine with the code you have now, that's why you have the WaitGroup. I just don't understand what the hijackHandler argument is for now, it will never be used by the net/http handler?

[gilwo]

You mean one returns from the handler and the other doesn't?

yes (for reference the projects are go-socket.io and socket.io)

Both cases should work fine with the code you have now, that's why you have the WaitGroup. I just don't understand what the hijackHandler argument is for now, it will never be used by the net/http handler?

notice the comment for the hijackHandler:

// hijackHandler is used for registering handler for connection hijacking, this is usefull for cases
// where there is no access to change the server KeepHijackedConns field (which is default as false)
// it also can be used for additional custom hijacking logic

in my code i used fiber as the web framework, fiber initialize KeepHijackedConns as false (default) and do not provide any mean to override this value see discussion reference here, this is why the handler exist as optional param.

is the go doc not clear enough, should it be revised ? 🤔

[erikdubbelboer]

@gilwo I have just pushed some changes to make it work how a net/http handler expects the Hijack method to work. Can you check if this works for you?

[gilwo]

@erikdubbelboer - the change appear to work seamlessly with my test app. this change seems much simpler, thanks!