[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 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 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 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 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 johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 @dcelasun: done, could you take another look? ---
[GitHub] thrift pull request #1488: Golang -remote.go client cleanup
GitHub user johnboiles opened a pull request: https://github.com/apache/thrift/pull/1488 Golang -remote.go client cleanup Several changes here related to the generated `-remote.go` clients * Serialize cli input with TJsonProtocol since json->thrift serialization is not supported with TSimpleJsonProtocol ([see also](https://github.com/apache/thrift/blob/19baeefd8c38d62085891d7956349601f79448b3/lib/go/thrift/simple_json_protocol.go#L335)) * Support for http headers (allows `Authorization: TOKEN` headers to be passed in the request) * Skip generation of `*-remote.go` for thrift that has no functions (they are not useful, and fail to build since `client` is unused) * Support `https` urls You can merge this pull request into a Git repository by running: $ git pull https://github.com/johnboiles/thrift golang-https-in-remote-client Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1488.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1488 commit 273d9404ff1eae3d3b5d76f66ac34646c9321d72 Author: John Boiles <johnaboiles@...> Date: 2018-01-31T23:10:46Z golang: support https from generated clients commit 09cc3fdd3e1041a31f6758d2ef2e40aea58061b9 Author: John Boiles <johnaboiles@...> Date: 2018-02-01T19:06:18Z golang: don't generate -remote.go clients when there are no functions commit 512eae1a61ccff83ae6d3f3a5cadee6e5e2cf7cd Author: John Boiles <johnaboiles@...> Date: 2018-02-01T19:32:03Z golang: support for http headers in -remote clients commit 5d9365fb32c9f167265af8c1be41e14d1f02c80b Author: John Boiles <johnaboiles@...> Date: 2018-02-01T21:35:34Z golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol ---
[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 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 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 #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 > For Thrift, PRs are not merged, just closed. You'll see a commit with you as the author but someone else as the committer, like these. Ah got it, good to know thanks! ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 Travis was green so i've squashed it! FYI for the future: GitHub can now automatically squash commits in PRs at merge time. If that feature is enabled for the repo it shouldn't be necessary for committers to squash their branch prior to merging, since everything can be squashed via the GitHub UI:  ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 Done ^. Thanks again for all your direction Dcelasun ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1461
Good call, so then:
```
func (p *BaseClient) Client_() thrift.TClient {
return p.c
}
```
?
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1461
This is a lot cleaner -- I like it. We still have the same issue with
tests, though, if the base service is in a separate package -- you can't access
the client as `.c` on the base service since it's lowercased.
Maybe we expose the client with a getter method?
```
func (p *BaseClient) C() thrift.TClient {
return p.c
}
```
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 You're correct that it's a service extending another service; tbh I missed that too (I'm not the author of the `.thrift` files. I'm also generally new to thrift so still sorting out what should be generated). So basically the logic would be to only add `c: thrift.TClient` to the `*Client` struct if `extends.empty()` in the generator? I'll take a look and see how hard that looks. ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1461
> Are you getting this second "Base" struct with 0.11 compiler? Because I
can't reproduce.
I've been working off the master branch, but I just tried generation with
0.11 (327ebb6c2b6df8bf075da02ef45a2a034e9b79ba) and it looks like it has the
same issue. Here is a direct copy-paste from my generated code with 0.11:
```
// Deprecated: Use NewUserService instead
func NewUserServiceClientFactory(t thrift.TTransport, f
thrift.TProtocolFactory) *UserServiceClient {
return {UserServiceBaseClient:
NewUserServiceBaseClientFactory(t, f)}
}
// Deprecated: Use NewUserService instead
func NewUserServiceClientProtocol(t thrift.TTransport, iprot
thrift.TProtocol, oprot thrift.TProtocol) *UserServiceClient {
return {UserServiceBaseClient:
NewUserServiceBaseClientProtocol(t, iprot, oprot)}
}
func NewUserServiceClient(c thrift.TClient) *UserServiceClient {
return {
c: c,
UserServiceBaseClient: NewUserServiceBaseClient(c),
}
}
```
---
[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 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 #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 @dcelasun any thoughts on fixing the tests? Looks like `tutorial/go/src/tutorial/tutorial.go` imports its `BaseClient` from another package. Since `c` isn't public in that other package, my modified initializer fails. Probably the simplest fix would just be to add a public accessor for `c`, (e.g. `func (p *BaseClient) C() thrift.TClient`) and us that public access in my modified initializer methods. What do you think? ---
[GitHub] thrift issue #1461: Golang: fix for (deprecated) New*ClientFactory and New*C...
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1461 > Why are they deprecated? I probably only overlook sth, so bear with me That's a great question. Looks like they were marked deprecated by @dcelasun in #1382 as a part of THRIFT-4285. That discussion predates my involvement in Thrift. > Do we have a JIRA Ticket for this? If not, could you create one? Thanks for the nudge, I was just being lazy. Created as THRIFT-4447 ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
Github user johnboiles commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1382#discussion_r160477076
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -1953,177 +1960,75 @@ void
t_go_generator::generate_service_client(t_service* tservice) {
f_types_ << indent() << "func (p *" << serviceName << "Client) "
<< function_signature_if(*f_iter, "", true) << " {" << endl;
indent_up();
-/*
-f_types_ <<
- indent() << "p.SeqId += 1" << endl;
-if (!(*f_iter)->is_oneway()) {
- f_types_ <<
-indent() << "d := defer.Deferred()" << endl <<
-indent() << "p.Reqs[p.SeqId] = d" << endl;
-}
-*/
-f_types_ << indent() << "if err = p.send" << funname << "(";
-bool first = true;
-
-for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) {
- if (first) {
-first = false;
- } else {
-f_types_ << ", ";
- }
- f_types_ << variable_name_to_go_name((*fld_iter)->get_name());
-}
-
-f_types_ << "); err != nil { return }" << endl;
-
-if (!(*f_iter)->is_oneway()) {
- f_types_ << indent() << "return p.recv" << funname << "()" << endl;
-} else {
- f_types_ << indent() << "return" << endl;
-}
-
-indent_down();
-f_types_ << indent() << "}" << endl << endl;
-f_types_ << indent() << "func (p *" << serviceName << "Client) send"
- << function_signature(*f_iter) << "(err error) {" << endl;
-indent_up();
-std::string argsname = publicize((*f_iter)->get_name() + "_args",
true);
-// Serialize the request header
-f_types_ << indent() << "oprot := p.OutputProtocol" << endl;
-f_types_ << indent() << "if oprot == nil {" << endl;
-f_types_ << indent() << " oprot =
p.ProtocolFactory.GetProtocol(p.Transport)" << endl;
-f_types_ << indent() << " p.OutputProtocol = oprot" << endl;
-f_types_ << indent() << "}" << endl;
-f_types_ << indent() << "p.SeqId++" << endl;
-f_types_ << indent() << "if err = oprot.WriteMessageBegin(\"" <<
(*f_iter)->get_name()
- << "\", " << ((*f_iter)->is_oneway() ? "thrift.ONEWAY" :
"thrift.CALL")
- << ", p.SeqId); err != nil {" << endl;
-indent_up();
-f_types_ << indent() << " return" << endl;
-indent_down();
-f_types_ << indent() << "}" << endl;
-f_types_ << indent() << "args := " << argsname << "{" << endl;
+std::string method = (*f_iter)->get_name();
+std::string argsType = publicize(method + "_args", true);
+std::string argsName = tmp("_args");
+f_types_ << indent() << "var " << argsName << " " << argsType << endl;
for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) {
- f_types_ << indent() << publicize((*fld_iter)->get_name()) << " : "
- << variable_name_to_go_name((*fld_iter)->get_name()) <<
"," << endl;
+ f_types_ << indent() << argsName << "." <<
publicize((*fld_iter)->get_name())
+ << " = " <<
variable_name_to_go_name((*fld_iter)->get_name()) << endl;
}
-f_types_ << indent() << "}" << endl;
-
-// Write to the stream
-f_types_ << indent() << "if err = args." << write_method_name_ <<
"(oprot); err != nil {" << endl;
-indent_up();
-f_types_ << indent() << " return" << endl;
-indent_down();
-f_types_ << indent() << "}" << endl;
-f_types_ << indent() << "if err = oprot.WriteMessageEnd(); err != nil
{" << endl;
-indent_up();
-f_types_ << indent() << " return" << endl;
-indent_down();
-f_types_ << indent() << "}" << endl;
-f_types_ << indent() << "return oprot.Flush()" << endl;
-indent_down();
-f_types_ << indent() << "}" << endl << endl;
if (!(*f_iter)->is_oneway()) {
- std::string resultname = publicize((*f_iter)->get_name() +
"_result", true);
- // Open function
- f_types_ << endl << indent() << "func (p *" << serviceName <<
"Client) recv"
- << publicize((*f_iter)->get_name()) << "() (";
+ std::string resultName = tmp("_result");
+ std::string resultType = publicize(method + "_result", true);
+ f_types_ << indent() << "var " << resultName << " " << resultType <<
endl;
+ f_types_ << indent() << "if err = p.c.Call(ctx, \""
--- End diff --
When using one of the now-deprecated initializers, this line will cause a
panic, since `p.c` is nil. The deprecated initializers create the `TClient` at
`p.BaseMyServiceClient.c`
---
[GitHub] thrift pull request #1461: Golang: fix for (deprecated) New*ClientFactory an...
GitHub user johnboiles opened a pull request:
https://github.com/apache/thrift/pull/1461
Golang: fix for (deprecated) New*ClientFactory and New*ClientProtocol
functions
Latest thrift:master can cause panics when using deprecated
`New*ClientFactory` and `New*ClientProtocol` functions. This happens because
both the Client and the BaseClient have an instance of a `thrift.TClient`. The
deprecated methods initialize the BaseClient's TClient, but other methods use
the Client's `TClient`.
For example, current thrift master generates structs like this
```
type MyServiceClient struct {
c thrift.TClient
*MyServiceBaseClient
}
type MyServiceBaseClient struct {
c thrift.TClient
}
```
And also a method like this:
```
func NewMyServiceClientFactory(t thrift.TTransport, f
thrift.TProtocolFactory) *MyServiceClient {
return {MyServiceBaseClient:
NewMyServiceBaseClientFactory(t, f)}
}
```
If that method is used, later calls to service methods will panic, since
`p.c` is nil (the actual client was stored in `p.BaseMyServiceClient.c`).
```
func (p *MyServiceClient) DoStuff(ctx context.Context, request
*DoStuffRequest) (r *DoStuffResponse, err error) {
var _args139 DoStuffArgs
_args139.Request = request
var _result140 DoStuffResult
if err = p.c.Call(ctx, "do_stuff", &_args139, &_result140); err != nil
{ // PANIC
...
```
The fix in this PR merely sets both instances of `TClient`. Though probably
a better fix would be to either remove these deprecated methods, or figure out
which `TClient` is the correct one to set.
I'm not sure which is right, so hoping to get some feedback/input here.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/johnboiles/thrift
golang-fix-broken-client-factory
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1461.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1461
commit d0c76c316c67241fc0e5add1e76e6128bf063ac2
Author: John Boiles <johnaboiles@...>
Date: 2018-01-08T21:53:43Z
Fix New*ClientFactory and New*ClientProtocol methods to prevent panics
commit fc87b4ad579e0f7a9f28ffc4a272f362cd2089ec
Author: John Boiles <johnaboiles@...>
Date: 2018-01-08T22:18:47Z
Fix formatting for New*ClientFactory and New*ClientProtocol methods
---
[GitHub] thrift issue #1459: Golang: do something with context.Context
Github user johnboiles commented on the issue: https://github.com/apache/thrift/pull/1459 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. ---
[GitHub] thrift pull request #1459: Golang: do something with context.Context
GitHub user johnboiles opened a pull request: https://github.com/apache/thrift/pull/1459 Golang: do something with context.Context 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 time out out based on `Context.Deadline`. 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. You can merge this pull request into a Git repository by running: $ git pull https://github.com/johnboiles/thrift johnboiles/go-context-http Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1459.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1459 commit 757bd5301dbfe923df6b4e21d1054fec4abc833f Author: John Boiles <johnaboiles@...> Date: 2018-01-05T22:37:05Z Golang: do something with context.Context (At least in http_client.go) ---
