Fix race condition in thrift client

[samikshya-db]

Description

• This issue was uncovered as part of bug raised in [BUG] - query that projects various columns intermittently throws Cannot invoke "org.apache.hive.service.rpc.thrift.TGetFunctionsReq.isSetOperationId()" because "req$9" is null #958. Although the issue was resolved as part of PR add a workaround for getFunctions in Thrift #979 - the cause is deeper than that - TCLIService.client causes state leakage in case previous server call
• This PR fixes the issue.
• Internal doc on root-cause and analysis: https://docs.google.com/document/d/12kciNIqy-kL1e0HOQs9JOjJlb2zWeNFl02e9kO772PE/edit?tab=t.0

Testing

• Reset code before this commit where we fixed getFunctions, and the following code no longer throws error

for (int i = 0; i < 10; i++) {
      try {
        DatabaseMetaData metaData = con.getMetaData();
        metaData.getFunctions("main", "testschema", null).close(); // Metadata call

        Statement stmt = con.createStatement();
        stmt.execute("select * from samikshyachand.default.hello_table"); // SQL query
        stmt.close();

        Thread.sleep(100); // Add a small delay
      } catch (Exception e) {
        e.printStackTrace();
      }
    }

Additional Notes to the Reviewer

[gopalldb]

nit: don't use caps for non final variables

[gopalldb]

we don't need this logic any more?

[gopalldb]

Samikshya explained to me offline, @jayantsing-db can you also take a look as you worked on this previously.

[jayantsing-db]

If i am understanding correctly, a JDBC connection will now have a single thrift client. So, when a JDBC connection is used in different threads concurrently, it will use the same thrift client. Now, if we don't put a sync mechanism, different threads using the same thrift client will fail because:

• thread A can increment the seq number of thrift client from x to x + 1
• thread B can receive the response for the seq number x. thread B will see that seq num is not matching and will throw exception: out of sequence response

This section of the code with all good intent and purposes tries to put a sync mechanism in place. But I am afraid, this might not be sufficient.

So the thrift calls happen in this order sequentially for a thread:

TCLIService.Client.ExecuteStatement -> seqid ++ -> transport.write -> transport.flush -> <wait for the http response> -> transport.read (when reading it checks the seqid with that present in message)

The current sync mechanism only protects transport.write and transport.read. And things could fail in exciting ways because the whole chain is not isolated/protected.

Happy to chat if this does not make sense at all.

[jayantsing-db]

Ideally, JDBC is a sync protocol and client app is not advised to use the same connection in different threads but there is no strict enforcement. Also i wrote a basic test on top of these changes that uses a JDBC connection across many threads and executes statements. I didn't encounter any errors (but i believe like other threading issues that's by chance that threads didn't step over each other)

[samikshya-db]

For this change, my goal was specifically to address the getFunctions issue raised in this repo. I may not be fully following the broader concern you mentioned — could you please share a concrete test case where the current fix fails? That would really help me understand it better.

[jayantsing-db]

I see that Samikshya has included a reproducer and these changes fix the reproducer while the earlier thread-local changes were failing. Do we have a understanding of why thread-local changes were failing and these are passing (in case there is any misunderstanding)? I can try to run the reproducer with earlier changes and these changes to understand more.

[samikshya-db]

@jayantsing-db

The failure path shows that the getFunctions call is constructing a TGetFunctionsReq with functionName:null -> and the server rejects it (Required field 'functionName' was not present!) -> This leaves the TCLIService.Client in a bad state, which then leaks into the subsequent ExecuteStatement call — hence the req$9 is null error showing up there as well.

The earlier thread-local changes didn’t help because they only isolated clients at the thread level, but the same TCLIService.Client object was still being reused within a connection and carrying over the bad state. With this PR, the client is properly reset between requests, so the reproducer consistently passes.

[samikshya-db]

Hello Gopal, Jayant !
I am merging the 2 following PR, but let me know if you have any additional feedback - I will address in follow up PRs. Thanks for the review!