Go 1.7 uses import "context"

[puellanivis]

If you're using the new "context" library, then gRPC servers can't match the server to the interface, because it has the wrong type of context.

Compiling bin/linux.x86_64/wlserver

asci/cs/tools/whitelist/wlserver

./wlserver.go:767: cannot use wls (type _Server) as type WhitelistProto.GoogleWhitelistServer in argument to WhitelistProto.RegisterGoogleWhitelistServer:
*Server does not implement WhitelistProto.GoogleWhitelistServer (wrong type for Delete method)
have Delete(context.Context, *WhitelistProto.DeleteRequest) (_WhitelistProto.DeleteReply, error)
want Delete(context.Context, _WhitelistProto.DeleteRequest) (_WhitelistProto.DeleteReply, error)

[iamqizhao]

1.7 has not been released. Let's worry about it later.

[puellanivis]

They have however released a beta test already. The whole idea behind pre-release versions is so that people can ensure compatibility for when it is released.

It's not like pulling context into the standard library is going to change. And whatever the eventual solution put into grpc is, it will still have to accommodate both Go 1.6.x and Go 1.7…

So, why not have the bug open but lower priority, and maybe someone could work on it occasionally ahead of Go 1.7 is released at which point, this bug becomes a significantly bigger priority, and would just have to be reopened/refiled anyways. I mean, release is scheduled for August, two months-ish away.

Why would you close this bug, when it's just going to be reopened two months from now?

[iamqizhao]

Because this will be buried in a lot of other things after 2 months and some ppl will create a new one (most ppl won't check if there is a duplicate already) when 1.7 is released and we need to dig out this "old" one and mark the new one "duplicated"...

I do not mind reopening this if you want anyways...

[puellanivis]

Maybe it's just the SRE in me, but I think projects should be prepared for change, rather than scramble once things are on fire.

We know this change is going to happen, and it has basically been on the horizon for a long time already. So, why not do something about it now?

Fix a bug before the vast majority of people even have a chance to file a ton of duplicate bugs? Priceless.

[iamqizhao]

Feel free to contribute.

[puellanivis]

Yeah, I'm thinking of how one could go about making it work for both golang.org/x/net/context (pre-1.7) and context (1.7) at the same time…

Kind of drawing blanks right now.

[crast]

@puellanivis The simple solution: Have people keep using golang.org/x/net/context - it already has provisions where it will use all the underlying implementations from go 1.7 context.Context if running in go 1.7.

You can pass a x/net/context.Context to a function expecting a context.Context because both of the interfaces have equivalent method sets. The only problem here is in satisfying the function signature from generated servers. What it means is go 1.7 users using GRPC will still need golang.org/x/net but this isn't really a huge deal.

You can compose contexts using e.g. WithTimeout WithCancel from both packages and it will do the right thing.

[crast]

^ fwiw, I've already tested the above on go 1.7beta1, and it works, and the advantage is the same code works on go 1.5, 1.6. 1.7

[puellanivis]

The problem is not the problem with being able to pass a context.Context into a function that takes golang.org/x/net/context.Context, it's that you can't use a Server with a receiver method func (s *Server) gRPCCall(ctx context.Context, in *pb.RequestProto) (out *pb.ReplyProto, error) into a function that wants an interface with a receiver method of gRPCCall(ctx golang.org/x/net/context.Context, in *pb.RequestProto) (out *pb.ReplyProto, error)

They don't match, because of Go's strong typing. Therefore, you need a mishmash of "golang.org/x/net/context" and "context" in different situations, where you use the x/net/context when you need to have a server match the method signature for the interface in gRPC, while you can use context elsewhere.

Yes, a work around is still “just use golang.org/x/net/context forever", but the whole reason for putting context into the standard library is that it's no longer experimental anymore and isn't confined semantically into the "net/" packages anymore.

[puellanivis]

What it means is go 1.7 users using GRPC will still need golang.org/x/net but this isn't really a huge deal.

Except now, because I have things that expect a context.Context receiver signature in the interface, and the gRPC one that expects a golang.org/x/net/context receiver signature in the interface.

So, now, I have to include two context packages, rename one of them, and one of them is obsoleted by the other. And then I have to remember which of the context packages I renamed, and remember if and when I didn't need to rename it because I'm only using one context type.

I made that stepping stone the same day this bug was filed, but it is not good programming.

When 1.7 releases, use of golang.org/x/net/context should be considered deprecated, and likely to disappear in 1.8, because that's how taking things out of the experimental tree typically works.

[crast]

I made that stepping stone the same day this bug was filed, but it is not good programming.

Conversely, a package which breaks version compatibility in weird ways for the sake of reducing package count isn't good package maintenance.

Yes, a work around is still “just use golang.org/x/net/context forever", but the whole reason for putting context into the standard library is that it's no longer experimental anymore and isn't confined semantically into the "net/" packages anymore.

"forever" is kind of over-blown, given how fast the Go ecosystem moves. If the trend shown in the last set of security fixes this spring hold, where the crypto vulnerability was only fixed for 1.5 and 1.6 (1.5.4 and 1.6.1) then the currently supported versions of go is the last 2 minor versions. At the current release rate of one every 6 months, this means that by February 2017, the only Go versions getting security updates will be 1.7 and 1.8. Anyone still on <=1.6 at that point who wants to run a server will move up.

I think it's completely sensible to keep using x/net/context until approx. February 2017, to facilitate the transition

[puellanivis]

Conversely, a package which breaks version compatibility in weird ways for the sake of reducing package count isn't good package maintenance.

Which is why I don't. My package github.com/puellanivis/protobuf/proto for instance doesn't break version compatibility. It uses "context" when go1.7 build tag is defined, and uses "golang.org/x/net/context" when go1.6 is defined.

I would expect my eventual solution for gRPC will do similar.

At the current release rate of one every 6 months, this means that by February 2017, the only Go versions getting security updates will be 1.7 and 1.8. Anyone still on <=1.6 at that point who wants to run a server will move up.

And if we fix it now, when people move from 1.6 to 1.7, they'll just replace "golang.org/x/net/context" with "context" and everything will continue to work. This would best be complete while 1.7 is the most recent release. Then at 1.8, drop support for 1.6.

I think it's completely sensible to keep using x/net/context until approx. February 2017, to facilitate the transition

But why wait until the last day? Why procrastinate? This is like the Y2K bug. We know it's going to happen, so why not fix it NOW, rather than in a rush before it's required?

I understand that you believe it to be sensible to keep using golang.org/x/net/context. However, not everyone need agree with you. I can work on this while other people work on other things, and eventually, integrate my patches (in a way that will not break version compatibility), and then people can switch over to using just "context".

This is a snowdrift, and I'm plowing it. One shouldn't be looking to stop me from plowing it by saying that we don't have to get to work until 17:02, and it's only 16:06, so like, “We have like an hour dude, we don't need to do it now.” I don't care, I'm clearing the snowdrift now. I can take my time and make sure it's done right the first time, rather than adhoc because last second.

[puellanivis]

BUT… I don't actually have to change any of the grpc-go files that are not defining types… which saves a lot of effort and/or duplication of code.

[griesemer]

@puellanivis Another work-around is to use a wrapper closure. Here's an example: https://play.golang.org/p/dbQc6HLGoV

[puellanivis]

Hm… could be useful… but it should work without changing anyone's code…

[puellanivis]

Turns out, it looks like just interceptor.go needs to be pulled out by build tag, and the type definition for methodHandler in server.go needs to be extracted into its own file to allow for different build tags.

With that, it works with go1.7…

Now, to test with go1.6…

[zellyn]

Sent an email referencing this issue: https://groups.google.com/forum/#!topic/golang-dev/lOqzH86yAM4

[puellanivis]

And it works. The only changes need be made are extracting out a few types to versioned build tag files, and a synchronous change to protoc-gen-go (which just changes one variable saying which context to use when generating code). Then code will compile with 1.6 using "golang.org/x/net/context" and it also compiles with 1.7 with the only change being s(golang\.org/x/net/context)(context)

gRPC keeps using golang.org/x/net/context where it doesn't cause a type conflict, but can be switched the same way as user's code (the s-regex) when supporting anything earlier than 1.7 stops making sense.

I'll work on gathering it up into something that can actually be a pull request. Plenty of time to get that done though. :)

[crast]

This is a snowdrift, and I'm plowing it. One shouldn't be looking to stop me from plowing it by saying that we don't have to get to work until 17:02, and it's only 16:06, so like, “We have like an hour dude, we don't need to do it now.” I don't care, I'm clearing the snowdrift now. I can take my time and make sure it's done right the first time, rather than adhoc because last second.

That's not what I was implying by saying this. I'm not saying don't plow the snowdrift. By no means should we not be forward looking... however, what I'm saying is: avoid knocking over telephone poles with your giant ice pusher, because some people really want their lights to work.

I'll use a really real example. I work for a company and we have a number of Go projects. Currently, we're doing continuous integration, testing, and actual deployment on Go 1.6.2, but a number of our developers are using 1.7beta1 locally, to be forward looking and make sure our stuff is ready for Go 1.7 as we're excited about moving over. That said, our code still needs to use x/net/context because we need to deploy on go 1.6 for the forseeable future (even when 1.7 gets released, there will likely be a lag time of a month or two before we fully roll it out in production, during which time we might have some 1.7 processes as canaries comingled with 1.6.2 processes).

If GRPC starts conditionally using context vs x/net/context depending on where it's built, it could force us to use build flag based interfaces everywhere in order for us to build with both versions, which leads to a fair amount of duplication. The only good thing is that this will be caught at the compiler level and by CI, so it won't make it to production, but it will significantly increase complexity for minor savings, given x/net/context still imports "context" in go 1.7 making it a fairly thin package from the binary level (yes I know, it still requires you checking out the considerably large x/net repo... but given that GRPC also needs x/net for other things it's mostly irrelevant)

[puellanivis]

Ok, I see what you're trying to say. The problem though is that fundamentally, we cannot make a “context” vs “x/net/context” agnostic system. People who more quickly move over to “context” will be broken unless we move some of the gRPC code over to “context”, but then others would have code break because they need the same source to be able to compile cross-versions.

There just isn't a way around this. Unless we use an interface{}, then type cast. But fundamentally they're still the same interface.

:( Really, the problem here is that we're having the system care about if it's context.Context or golang.org/x/net/context.Context, when both of the interface definitions are identical (baring comments and whitespace).

With protoc-gen-go, it's easy enough to switch between the two as the target context library is a variable that can be swapped by option flags/whatever.

Unfortunately, in gRPC, this isn't feasible… unless we use interface{} in the function definitions and just assert type. Which, I DO suppose is a possible option…

Alternatively, we could just use:
type Context interface {
context.Context
}

And then the function signatures use grpc.Context, but everything else can go ahead and just use context.Context or golang.org/x/net/context because they'll all assert type without any possible panic. (Due to the identical interfaces)

But even with interface{}, we're screwed, because people would have to change their code to whichever context they're using over to interface{}…

Ugh… this problem…

Last idea, we just use interface{} on the function types, and then type assert it to whichever context it's actually defined as using… then the same code would work for both !go1.7 and go.17… and then, no code change on the external-to-gRPC side.

[griesemer]

FWIW, we are working on a related issue which is a language mechanism that allows us to say that X declared in package p1 is really the same X (p2.X) as declared in package p2; i.e., a mechanism to declare one object (type, variable, function) as an alias of another one. An alias is simply an alternate name for the same object. This is becoming very important if one wants to evolve systems with large numbers of dependencies which cannot all change at the same time.

We will post an official proposal soon - probably after the 1.7 release candidate is out.

[crast]

@puellanivis You can actually handle it in the grpc codegen with some sort of option or flag to the codegen.

if you wrote a method called Ping, you would get a method handler something like this:

func _MyService_Ping_Handler(srv interface{}, ctx context.Context, dec func(interface{}) error, interceptor grpc.UnaryServerInterceptor) (interface{}, error) {
    in := new(PingRequest)
    if err := dec(in); err != nil {
        return nil, err
    }
    if interceptor == nil {
        return srv.(MyServiceServer).Ping(ctx, in)
    }
    // [...]
    // omitted because it would be solved similarly
}

It's actually already generating an adaptor for every function anyway, to call each individual method as needed but provide a single function call interface to the GRPC server setup. it's this _MyService_Ping_Handler that is actually called before it calls your server implementation's Ping method.

If you were to make a tweak to the code-gen that when some flag is set, it would generate code like this:

import (
    context "context"
    netcontext "golang.org/x/net/context"
)

// [ ... ] other code-gen stuff

type MyServiceServer interface {
    Ping(context.Context, *PingRequest) (*PongResponse, error)
}

func _MyService_Ping_Handler(srv interface{}, ctx netcontext.Context, dec func(interface{}) error, interceptor grpc.UnaryServerInterceptor) (interface{}, error) {
    in := new(PingRequest)
    if err := dec(in); err != nil {
        return nil, err
    }
    if interceptor == nil {
        return srv.(MyServiceServer).Ping(ctx, in)
    }
    // [...]
    // omitted
}

You'll notice the function body is almost identical, except that the method uses netcontext.Context and the interface containing Ping uses context.Context. This would actually work and compile, because each Context's method set is a strict subset of the other (it's directly assignable without a type assert).

Advantages:

1. Allows user to choose how they use context, without conditional compilation stuff
2. Solves this in a way that allows go 1.7 users to keep using x/net/context if they need it (like compiling for multiple versions)

Disadvantages:

1. You still need to import x/net/context even on go 1.7, for use inside your .pb.go file and GRPC uses x/net/context internally everywhere (though your project code could be free of x/net/context)
2. Maybe more complexity to the codegen to support both modes, as you can't do this with a simple regex.

[puellanivis]

a) I've given up on throwing out x/net/context entirely. It's going to need to stay there until 1.6 is no longer a reasonable option.

It seems on its face to be good, but something just nags me. Oh, grr… I still can't make a server with RPCs that point to the other context. Sure, people are unlikely to mix-and-match in the same source, but at some point, someone will hit the “cannot use func(context.Context) as func(context.Context)” error, and too many people would probably not be able to understand what's broken there (because what do you mean you can't use that signature as the apparently same signature?)

:/ Lots of possible solutions, but they all kind of suck one way or the other.

[aaronjheng]

golang/protobuf#217

[johanbrandhorst]

Bump, is this possible now that we have aliases in 1.9?

[dfawley]

If you are on Go 1.9, is this even an issue for you? If so, please explain why.

Otherwise, I believe this should only be changed when we drop support for Go 1.8 - i.e. all grpc-go supported versions have type aliases. If we change it before then, we will break interactions between old pb.go files and grpc-go.

[johanbrandhorst]

Just to make it clear for us on 1.9 - whats required in the code to use context and golang.org/x/net/context interchangeably?

[dfawley]

AIUI x/net/context for Go1.9 builds is a type alias to context. So if you (or anything else) import x/net/context everything should "just work".

[dfawley]

(See: https://github.com/golang/net/blob/master/context/go19.go)

[jonathaningram]

@dfawley if it helps, this is mostly resolved for my project after upgrading to 1.9, except that GAE standard will probably not push out 1.9 that soon so for those projects of mine using GAE, the problem still exists.

[dfawley]

Sigh. My understanding is GAE is supposed to get 1.9 much faster than it took to get the 1.8 release.

I would love to get rid of the x/net/context import, but we have to balance this inconvenience with breaking everyone using existing .pb.go files.

For people stuck using older versions of Go, 1.8 introduced a fix feature for context. go tool fix -force context <src path> should update everything quickly.

[edmund-troche]

Have we considered starting a 2.x branch which would have the new context? Usually a change in the version major implies a break in compatibility, so it would not be a surprise for someone moving to 2.x. There could be some parallel development going on 1.x and 2.x during a deprecation period and then eventually is just 2.x.

I'm coming in late to this discussion, so maybe I've missed previous discussions along the lines of what I've suggested, also maybe what I'm suggesting is just not feasible, but just curious why/if this would be a reasonable approach.

[Ulexus]

@edmund-troche For just this issue, that would be far overkill. With Go 1.9, this has mostly turned into a non-issue.

[dfawley]

FYI, support for Go 1.6-1.8 will be removed on or after November 1st, and "context" will be imported directly at that time. This will be safe because "x/net/context" is a type alias in 1.9 and later.

[puellanivis]

From x/net/http2: (link)

21 more days...
https://groups.google.com/forum/#!topic/google-appengine-go/wHsYtxvEbXI

We’re writing to let you know that we are deprecating Go 1.8 support on App Engine, and new deployments using this language version will no longer be available on November 1, 2018.

And golang/protobuf: (link)

Here's a convenient countdown clock.