THRIFT-4448: Golang: do something with context.Context

[johnboiles]

This patch wires through context.Context such that it can be used in in http.Request's WithContext method. This allows Thrift HTTP requests to canceled or timed out via the context.

This patch breaks support for go<1.7 so it's not ready to ship, but I'm hoping to get some direction on this. When does Thrift expect to drop support of go1.7? It looks like the current solution is to duplicate files that need to use golang.org/x/net/context and add a // +build !go1.7 but duplication seems unsustainable as the context package is imported more places.

Go 1.7 was released 15 August 2016. Given Golang has had significant performance improvements in most dot releases, I suspect most production services stay reasonably up to date. Here at Periscope/Twitter we're on go1.9.1, and we're a fairly large organization.

[almalkawi]

Please run go imports to fix imports order. Looks great!

[almalkawi]

LGTM

[johnboiles]

Looks like two of the builds failed as expected on go1.6 context. The third build that failed looks like it failed on JS issues, so likely something flakey.

[dcelasun]

When context support was added, significant effort went into keeping 1.6 compatibility so I think it would be great if we could maintain that support.

On the other hand, this patch can go into 0.12 at the earliest and by that time 1.6 will be ancient. I haven't reviewed the patch yet, but if you'd like to drop compatibility, please do so across the codebase (so no more x/net/context anywhere).

[johnboiles]

I really appreciate your feedback & involvement @dcelasun! I personally think dropping go1.6/x/net/context support is best for the project. Are you the right person to make that call? Or are there others we should check with before I put in the work.

Related question: why was 'context support' added when it doesn't seem to be hooked up to anything in the library? Seems like the method signatures were the only thing that changed.

[dcelasun]

I personally think dropping go1.6/x/net/context support is best for the project. Are you the right person to make that call?

Sounds good to me, but we need to run it by someone with commit bits. cc @Jens-G @jeking3

Related question: why was 'context support' added when it doesn't seem to be hooked up to anything in the library? Seems like the method signatures were the only thing that changed.

It was added to support passing arbitrary values into RPC handlers. One of the big use cases was accessing client IP from an RPC handler. Before context, this was impossible since the underlying transport was not visible to the handler. Now, you can implement a custom TProcessor that embeds the generated processor, and pass the IP manually, something like:

type MyProcessor struct {
    *GeneratedProcessor
}

func (p *MyProcessor) Process(ctx context.Context, in, out TProtocol) (bool, TException) {
	name, _, seqId, err := in.ReadMessageBegin()
	if err != nil {
		return false, err
	}
	if processor, ok := p.GetProcessorFunction(name); ok {
		//TODO: get IP from transport and add to ctx
	
		return processor.Process(ctx, seqId, in, out)
	}
	
	// rest of it...
}

[johnboiles]

Thank you for the explainer @dcelasun! Makes sense to me.

@Jens-G, @jeking3: if one of you sign off on this, I'll make the changes to drop go1.6 and x/net/context support from thrift:master.

[jeking3]

At this point we should consider dropping go 1.6. Other go projects have moved on and we can too. I had a discussion with someone about whether it makes sense to support older go versions, and they said back to 1.7 makes sense, but 1.6 to 1.7 was a breaking change. Separately, we might dump the trusty CI build environment. Not sure about that yet.

[johnboiles]

Sounds good want me to take a stab at dropping 1.6 support?

[jeking3]

Ugh, so it turns out Ubuntu Xenial comes with go 1.6 which is pretty much the version we use on all of our CI builds for testing (like cross test).

For further details on the issue see:

golang/mock#118

Now I have to think some more about it. :|

[jeking3]

My thoughts after sleeping on it are that we should support the current Ubuntu LTS. That said, I have no problem telling folks not to use the ancient golang included in the LTS and to use something newer from either an Ubuntu PPA or directly form golang sources.

I also found evidence in a number of other projects that support for golang 1.6 was dropped. So if you want to fix this in the way suggested, you will need to update the ubuntu-xenial Dockerfile to use golang 1.7 instead, and you should update the top level LANGUAGES.md file, add a breaking change note into the lib/go README file, and update the build/docker/README.md file. Thanks.

[johnboiles]

I think that's the right approach. It’s pretty easy/fast to install golang from a tar.gz distribution. I do it pretty frequently to pin a specific version of go in docker images.

I'll take a stab at updating this hopefully tomorrow or early next week.

[johnboiles]

@jeking3 @dcelasun Travis is green so I think we're almost good to go here. Mind taking another look and merging if it looks good?

[dcelasun]

It would be nice to add a client test using e.g context.WithTimeout(). Something like:

• In the handler function, sleep for 100ms before returning
• In the client, call the RPC with context.WithTimeout(context.Background(), 50*time.Millisecond)
• Check if ctx.Err() == context.Canceled

Otherwise LGTM.

[johnboiles]

@dcelasun I'm happy to do that. Though in my experience, tests that depend on timing will find a way to be flakey, especially in CI.

If you think the value of testing this is worth the risk of introducing a flakey test, I'm happy to write it.

[dcelasun]

I think it's worth it. The context passes through most parts of the stack and having a test to ensure it's propagated properly feels imporant.

Also, we can at least reduce the flakiness with longer timers, say 2s sleep and 1s timeout. The test suite already takes quite a bit of time to run, an additional second should be fine.

[johnboiles]

@dcelasun: done, could you take another look?

[dcelasun]

The test fails with:

src/common/context_test.go:45: server.Close undefined (type *http.Server has no field or method Close)

Server.Close() was added in 1.8 but the test uses 1.7. You can just remove that line since it's not a long running process, or you can make it conditional with runtime.Version.

[johnboiles]

@dcelasun I don't think I can do a runtime.Version check since the code is failing at compilation time. I'll just remove that line.

[dcelasun]

That’s what I get for reviewing code before fully waking up :)

[dcelasun]

A different failure this time:

--- FAIL: TestHttpContextTimeout (0.00s)
	context_test.go:74: Unexpected error: dial tcp 127.0.0.1:9096: getsockopt: connection refused

[johnboiles]

Hmm yeah must be a race condition with the test server starting in the go routine. Any thoughts on how to wait for it to start up? I guess I could use a wait group to wait at least until goroutine code begins to execute. Or I could have some retry logic that tries to contact the server for a second before failing

[dcelasun]

I think the only one-liner fix is a 500ms sleep before client.TestVoid(). Retry logic would work as well.

[johnboiles]

It's not clear to me what went wrong in the Travis build. Trusty, for example, seems to have stack overflow'd while installing ocaml. Is there a way to retrigger a build without pushing another commit?

[default] synchronized from https://opam.ocaml.org
Stack overflow
[ERROR] Initialisation failed
Fatal error:
Stack overflow
The command '/bin/sh -c apt-get install -y --no-install-recommends `# OCaml dependencies`       ocaml       opam &&     opam init --yes &&     opam install --yes oasis' returned a non-zero code: 1

[dcelasun]

The only other way is for someone with Travis crendentials to manually trigger it. I think it's faster if you push something trivial (whitespace etc.)

Thrift's CI builds are unfortunately very hit and miss :/

[dcelasun]

LGTM. Travis failures are unrelated.

cc @jeking3

[johnboiles]

It bugged me to have a time.Sleep in the test so I wrote the retry logic :)

[dcelasun]

Looks much cleaner!

[jeking3]

Would you mind squashing this into a single commit?

[johnboiles]

@jeking3 done