Transports and contexts

[Ayiga]

remove creation and call of server.Serve method's context.WithCancel.

In this initial commit I believe it would be in the interest of flexibility to not force the creation and immediate cancellation of the context that is spawned from the server.

This cancellation prevents the ability to fire an asynchronous request that inherits from the same context that was given for the Serve request.

However, I believe that may warrant further discussion, as in the golang 1.7 release there will be a context that will likely be inherited from in a future change to go-kit, that will cancel the underlying context when the client's connection is closed:
https://github.com/golang/go/blob/master/src/net/http/server.go#L1508-L1509

This is also the case with client-side connection as well in the new DialContext method
https://github.com/golang/go/blob/master/src/net/dial.go#L308-L309

In both of these cases the context lives for the life-time of the connection. However, there is an important distinction. On the client side, the context is inherited from the context that is passed in. This means that when the context is cancelled, it will only cancel down the context inheritance stack, which is only the context that they've created. However, in the server side implementation, everything is inherited directly from the Connection handling / delivery with golang's net/http conn.serve method.

For Example the output of the following program

package main

import (
    "context"
    "fmt"
    "sync"
)

func WaitTilDone(ctx context.Context, wg *sync.WaitGroup ) {
    select {
    case <- ctx.Done():
        fmt.Println( ctx.Err() )
    default:
        fmt.Println("Not Canceled")
    }

    wg.Done()
}

func main() {
    wg := new(sync.WaitGroup)
    ctx1 := context.Background()
    ctx2, cancel := context.WithCancel(ctx1)
    wg.Add(2)

    // cancel ctx2 immediately
    cancel()

    go WaitTilDone(ctx2, wg)
    go WaitTilDone(ctx1, wg)

    wg.Wait()
}

is the following:

Not Canceled
context canceled

Perhaps this means that overall context utilization may need to be reconsidered on the Server, as any inheritance from the net/http's conn.serve method's context will be canceled and invalidated on client connection close.

In the mean time, I'd also like to consider the removal of the context extension / cancellation in the client transport methods as well. However, these hardly have much of a bearing, and I don't think they affect logic / asynchronous calls one way or another.

https://github.com/go-kit/kit/blob/master/transport/grpc/client.go#L77-L78
https://github.com/go-kit/kit/blob/master/transport/http/client.go#L80-L81

[Ayiga]

Something I feel I should point out, is that in the case of streaming, which is something you had brought up earlier, the underlying http connection is hijacked (in the case of changed protocols) and in those cases the Server's context will be cancelled as well:

https://github.com/golang/go/blob/master/src/net/http/server.go#L1561-L1563

[peterbourgon]

This is just pending the same changes applied to gRPC, if I understand correctly.

[Ayiga]

From what I see, this is targeting all of the Serve methods in each of the transports. Did I miss one?

Or are you referring to the two separate contexts that are being used currently in the ServeGRPC? Should we unify them, so that everything is just using the context that is passed in?

[peterbourgon]

Ah! You're right. I'm silly. Thank you!