Unable to initialize a websocket ping-ponger before the first `.Send()`

[LandonTClipp]

I asked this question in the examples repo but thought I would restate it here for visibility.

I have a websocket endpoint that streams server-side messages. The client will send nothing beyond pong messages. I am running into an issue where if the server has no messages to send when the websocket connection is first established, I cannot initialize the pingPonger goroutines because of how the generated code works. Here is our generated code:

// Send streams instances of "console.SubscribeResult" to the "subscribe"
// endpoint websocket connection.
func (s *SubscribeServerStream) Send(v *console.SubscribeResult) error {
	var err error
	// Upgrade the HTTP connection to a websocket connection only once. Connection
	// upgrade is done here so that authorization logic in the endpoint is executed
	// before calling the actual service method which may call Send().
	s.once.Do(func() {
		var conn *websocket.Conn
		conn, err = s.upgrader.Upgrade(s.w, s.r, nil)
		if err != nil {
			return
		}
		if s.configurer != nil {
			conn = s.configurer(conn, s.cancel)
		}
		s.conn = conn
	})
	if err != nil {
		return err
	}
	res := v
	body := NewSubscribeResponseBody(res)
	return s.conn.WriteJSON(body)
}

You can see that both the connection upgrader and the configurer are only run during the first .Send(). The obvious issue with this is that the lack of ping-pong messages causes the connection to go stale. More specifically, we use Cloudflare as an intermediary and it will eventually fail the connection with a gateway timeout error because of the lack of activity on the TCP connection.

Our only solution we've been able to find is to send an empty message through the connection in order to trigger the upgrader and configurer. This is non-ideal because it means the client receives an irrelevant message purely to workaround this "bug". Are there any other solutions to this that I am missing?

[raphael]

That makes sense, we ran into similar issues due to this behavior. I believe @xeger had some ideas on how to make this better.

[LandonTClipp]

Interested to hear what you guys think. I'm also happy to collaborate on a PR if there are no good existing solutions. I'm thinking of some pre-subscribe hook that users can define to automatically run.

[xeger]

One thought that I had was to expose the "post-connect-pre-send" action as a method of the stream struct itself.

Let's imagine that the code guarded by s.once.Do could be invoked by a func Prepare() of the stream.

Endpoints that need to "eagerly upgrade" (or whatever the equivalent is in GRPC) can call Prepare proactively before entering their streaming loop. Endpoints that can tolerate lazy upgrade, or that don't care, will continue to experience the existing lazy upgrade behavior.

I'd thought about adding some sort of hook too -- perhaps even using a streaming interceptor -- but as long as Send causes a socket message to be sent, there's no way to avoid application-level keepalives which are ugly.

What would Prepare look like for GRPC streams? Would it be a no-op?

[LandonTClipp]

Some kind of Prepare() function would be great. All we really need is a way to start these connection management goroutines and everything else would work. It does seem that if a user were to make use of Prepare() that we would want to unconditionally call the s.once.Do gate inside of that hook. The resulting conn would need to be passed to the hook.

As far as GRPC streams, I am less familiar with the protocol so couldn't speak as to what it would need. Keeping it as a no-op until someone could answer that question sounds like the safest route.

[xeger]

Are the Configurer and Upgrader interfaces sufficient for you to implement your hooks without Prepare needing to return something to you? In my mind, it was simply a way for the owner of the stream to ask for proactive setup and I'd hate for it to leak protocol details such as a websocket conn.

[LandonTClipp]

Are the Configurer and Upgrader interfaces sufficient for you to implement your hooks without Prepare needing to return something to you?

I think so? The hook would only need access to the websocket connection which is provided in the goahttp.ConnConfigureFunc function.

Just so I understand what your vision is, you're suggesting to add a .Prepare() method to the *ServerStream struct in the gen/http/${service_name}/server/websocket.go file. With that, we would define our service method (the thing implementing the endpoint) like this:

func (s ConsoleService) Subscribe(ctx context.Context, payload *console.SubscribePayload, stream console.SubscribeServerStream) error {
  stream.Prepare()
  {...}

The .Prepare() would do something like this:

func (s *SubscribeServerStream) Send(v *console.SubscribeResult) error {
	s.once.Do(func() {
		var conn *websocket.Conn
		conn, err = s.upgrader.Upgrade(s.w, s.r, nil)
		if err != nil {
			return
		}
		if s.configurer != nil {
			conn = s.configurer(conn, s.cancel)
		}
		s.conn = conn
	})
        {...}

The genhttp.New constructor would remain the same. Does that sound about right? If so, I think this would be a good solution. The unfortunate thing is that there is a small amount of boilerplate users have to add in the fact that we need to now call stream.Prepare() in every websocket endpoint. Perhaps that could be fixed by instead having the generated code itself call .Prepare(), but then that presents the question of how do you configure the generated code to do that in a way that is backwards compatible?

[MichaelUrman]

Would refactoring the s.once.Do bits into a Prepare() error helper, and invoking it between auth and the endpoint call do the trick? It needn't be part of the generated stream interface; instead the generated endpoint wrapper could check for it directly (presumably only for cases where it's expected). If I'm reading correctly the stream shouldn't even need s.once after that.

Does that cover all the compatibility concerns?

• It would always be called once when it's there
• Existing ...Stream test doubles would just be passed through
• Test doubles that happened to implement Prepare() error could be a concern, receiving a surprise call
  • If that's bad enough, other names (Prepare<Method>ServerStream, Configure<Method>ServerStream, etc.) might reduce the odds sufficiently, or internal types in the signature could make it impossible

If this would work, none of the risks sound like worse compatibility concerns than the recent addition of ...WithContext to the grpc stream interfaces.