Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-4447

Golang: Panic on p.c.Call when using deprecated initializers



    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.11.0
    • 0.12.0
    • Go - Compiler
    • None
    • Patch Available


      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
      type MyServiceBaseClient struct {
      	c thrift.TClient

      And also a method like this:

      func NewMyServiceClientFactory(t thrift.TTransport, f thrift.TProtocolFactory) *MyServiceClient {
            return &MyServiceClient{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

      In progress fix here :https://github.com/apache/thrift/pull/1461. The fix in this PR merely sets both instances of TClient (which is what happens in the non-deprecated New*Client function).

      This patch currently fails make -k check however, since src/tutorial/tutorial.go tries to access a different package's version of the BaseClient.

      src/tutorial/tutorial.go:477:33: bc.c undefined (cannot refer to unexported field or method c)

      The fix for that test could possibly be to expose the BaseClient's instance of c (by making it a capital and thus exported C), or adding an accessor method C() or Client().

      Possibly 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 preferred, so I'm hoping to get some feedback/input here.


        Issue Links



              jking3 James E. King III
              johnboiles John Boiles
              0 Vote for this issue
              3 Start watching this issue