Support evented read

This feature will avoid the need to dedicate a goroutine to read on the connection and enable read buffer pooling.

See eranyanay/1m-go-websockets for memory reduction possible with this feature.

See Go #15735 for discussion about network connection read events.

[eranyanay]

@garyburd @elithrar @stevenscott89
I'd like to address this and send a PR.

The problem here is mainly the implementation approach, which doesn't expose a way to read a single frame

If we want to support the usage of this library together with asynchronous IO mechanisms (select/epoll), we should introduce one of the following two changes that comes to mind:

1. Introduce a new function func (c *Conn) NextFrame() (messageType int, r io.Reader, err error)
   which will behave very similar to NextReader, but the inner call to advanceFrame will read the next frame, do most of the logic until the 7th step, where it processes the control messages on its own. If those are ping/pong control messages, the NextFrame flow will return at this point with the entire message. The regular flow for NextReader is kept with no changes
2. Keep the Conn struct exposed functions unchanged. In this solution we can change the Upgrader struct itself, and introduce a new member, AllowReadControlMessages bool (will come up with a decent name), which is then handed over to the newConn call, to let it know the operation method. This variable is then used during the NextReader and advanceFrame to decide whether to handle ping/pong messages internally, or expose them to the caller.

What's your thoughts on this?
I'd be happy to contribute this feature and help maintaining it over time.

The design of the standard library evented read API may determine API design choices in this package. Any change to the exported API to this package should wait on the standard library design.

If the standard library provides an edge trigger, then all that's needed is a new error value and a way to opt in to evented read:

// run is called on each edge trigger on c.ws.UnderlyingConn()
func (c *AppConn) run() {
    for {
        mt, r, err := c.ws.NextReader()
        if err == websocket.WaitIO {
             return
        } else if err != nil {
             c.cleanup(err)
        }
        // process mt, r
    }
}

Given that the switch from synchronous read to evented read necessitates changes to an application, the addition of the evented read is an opportunity holistically design an API that addresses other issues with this package. These issues are detection of dead connections and clean close of the connection. There are open issues on both of these topics. Although these features only require a few lines of application code, those few lines of application code are difficult to figure out. This is just spitballing here, but the holistic design may be to shift control over read from the application to this package by delivering data messages and connection closed notifications to the application as callbacks. Note that ping, pong and close messages are already delivered as callbacks. With this additional control, the package can implement the features for the application with little or no exposed API surface area to the application.

Even if the holistic design change in the previous paragraph is not considered, the detection of dead connections must be considered as part of the evented read API. The examples in this package use read with deadline to detect dead connections. What replaces that in the evented world? The replacement may not need any API changes, but we should know what that replacement is before committing to an API.

[eranyanay]

The standard library evented read API is not exposed at the moment and according to the issue there, doesnt seems like anything is going to change in that matter in the near future.

I dont think the approach of this library to perform epoll mechanism on each and every socket separately is a good idea, since it means the user will either:

1. Keep using 1goroutine per socket
2. Have a dedicated goroutine that calls ReadMessage() on all sockets, one at the time, performing epoll systemcalls as the number of open websockets

I think you refer to option 2, and it sort of contradicts the usage of polling mechanisms, where with a single call you can ask the condition of all the open sockets.

The whole point of asynchronous read APIs is that you perform them on multiple connections at once

Also note that performing it internally, how would you configure timeouts?
The beauty when using epoll/select is that you can dedicate one goroutine that calls the systemcall with infinite timeout, and in your suggested approach to hide it in the package we would still need some sort of timeout mechanism. and on connections with very few data, the suggested psuedocode will perform busy wait

I dont think that this logic should be part of the package logic. Its application logic.

Another thing to note, is that its a solution that is only applicable to Linux, and this library is at the moment functional for other OS's too. Leaving it for the user to decide is probably the better approach

Regarding detecting dead connections, I couldnt find any other open issues that directly says that, can you explain? With the Close handler its pretty much working as far as I can tell, and also calling ReadMessage on a closed socket returns an error that can be checked. So Im not sure what else is there to do in that aspect

Regarding detection and actual usage from outside, I think simply exposing the connection file descriptor is enough (do some reflection on Go Conn interface and add a function ws.Fd()), and then use it in application space together with epoll logic.

This package currently uses the Go standard library for reading the network. The standard library does not perform a separate epoll on each and every socket.

My suggested run method is called in response to an edge trigger and reads messages until there is no data available. The design assumes that the standard library will provide a single poller and that there's a way to invoke a function in response to an IO trigger. The design also assumes that the standard library will provide a way to toggle a connection between blocking and non-blocking read. These design assumptions are based on discussion in the issue linked above.

I am not following the discussion on Linux vs. other platforms. I am assuming that the standard library will provide a cross platform evented read API.

The CloseHandler is called in response to a received close message. That does not help with detecting dead connections. ReadMessage in the evented world is called when data is received. What happens when no data is received?

The most recent open issue (492) is about detecting dead connections.

[garyburd]

I do not have time to review this large feature, nor do I want the additional responsibility of maintaining the project with the feature. The feature will need to wait for a new maintainer.

[nhooyr]

I don't think it's a good idea to add this feature to this library. It's a very unidiomatic way of writing Go and anyone who wants it can always use gobwas/ws instead.

If Go adds the evented read API required for this feature, will evented read still be unidiomatic?

[iluozhiqiang]

@garyburd @elithrar @stevenscott89
I'd like to address this and send a PR.

The problem here is mainly the implementation approach, which doesn't expose a way to read a single frame

If we want to support the usage of this library together with asynchronous IO mechanisms (select/epoll), we should introduce one of the following two changes that comes to mind:

1. Introduce a new function func (c *Conn) NextFrame() (messageType int, r io.Reader, err error)
   which will behave very similar to NextReader, but the inner call to advanceFrame will read the next frame, do most of the logic until the 7th step, where it processes the control messages on its own. If those are ping/pong control messages, the NextFrame flow will return at this point with the entire message. The regular flow for NextReader is kept with no changes
2. Keep the Conn struct exposed functions unchanged. In this solution we can change the Upgrader struct itself, and introduce a new member, AllowReadControlMessages bool (will come up with a decent name), which is then handed over to the newConn call, to let it know the operation method. This variable is then used during the NextReader and advanceFrame to decide whether to handle ping/pong messages internally, or expose them to the caller.

What's your thoughts on this?
I'd be happy to contribute this feature and help maintaining it over time.

public NextFrame is my need