[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 @jeking3 done ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1459 Would you mind squashing this into a single commit? ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 Looks *much* cleaner! ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 It bugged me to have a time.Sleep in the test so I wrote the retry logic :) ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 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 :/ ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 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 ``` ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 I think the only one-liner fix is a 500ms sleep before `client.TestVoid()`. Retry logic would work as well. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 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 ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 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 ``` ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 @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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 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`](https://golang.org/pkg/runtime/#Version). ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 @dcelasun: done, could you take another look? ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 @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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 @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? ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1459 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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1459 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: https://github.com/golang/mock/pull/118 Now I have to think some more about it. :| ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 Sounds good want me to take a stab at dropping 1.6 support? ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1459 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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
> 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:
```go
type MyProcessor struct {
*GeneratedProcessor
}
func (p *MyProcessor) Process(ctx context.Context, in, out TProtocol)
(bool, TException) {
name, _, seqId, err := iprot.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, iprot, oprot)
}
// rest of it...
}
}
```
---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 When context support was added, significant effort went into keeping 1.7 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.7 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). ---
