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

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

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.11.0
    • Fix Version/s: 0.12.0
    • Component/s: Go - Compiler
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      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 &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.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                jking3 James E. King III
                Reporter:
                johnboiles John Boiles
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: