Thrift
  1. Thrift
  2. THRIFT-945

TAsyncClient class's currentMethod is never set, hence a second call on the same client will fail if a previous call is ongoing.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.4
    • Fix Version/s: None
    • Component/s: Java - Library
    • Labels:
      None

      Description

      TAsyncClient has this check:

      protected void checkReady() {
      // Ensure we are not currently executing a method
      if (currentMethod != null)

      { throw new IllegalStateException("Client is currently executing another method: " + currentMethod.getClass().getName()); }

      However currentMethod is not being set anywhere.
      In my code I have a TAsyncClient and method calls made on it are not necessarily serialized (one starts after previous finishes), so interleaving calls will fail with mysterious messages such as:

      java.lang.IllegalArgumentException
      at java.nio.ByteBuffer.allocate(ByteBuffer.java:311)
      at org.apache.thrift.async.TAsyncMethodCall.doReadingResponseSize(TAsyncMethodCall.java:175)
      at org.apache.thrift.async.TAsyncMethodCall.transition(TAsyncMethodCall.java:128)
      at org.apache.thrift.async.TAsyncClientManager$SelectThread.run(TAsyncClientManager.java:99)

      1. thrift-945.patch
        3 kB
        Bryan Duxbury

        Activity

        Hide
        Eric Jensen added a comment -

        it's a common misunderstanding that this client could possibly support concurrent calls. it cannot since it is backed only by a single socket just like the blocking Client calls. if you want concurrent calls you must pool these clients or their sockets yourself.

        Show
        Eric Jensen added a comment - it's a common misunderstanding that this client could possibly support concurrent calls. it cannot since it is backed only by a single socket just like the blocking Client calls. if you want concurrent calls you must pool these clients or their sockets yourself.
        Hide
        Bryan Duxbury added a comment -

        This patch adds a test and appears to fix the issue. Thoughts?

        Show
        Bryan Duxbury added a comment - This patch adds a test and appears to fix the issue. Thoughts?
        Hide
        Eric Jensen added a comment -

        oh, the title of this ticket confused me. the bug we are really fixing is that it fails with the wrong, uninformative, message if you try to make a concurrent call. do we really intend for these to not be reusable? i think the only problem we should protect against is using them for two concurrent calls as opposed to making them non-reusable. ning?

        Show
        Eric Jensen added a comment - oh, the title of this ticket confused me. the bug we are really fixing is that it fails with the wrong, uninformative, message if you try to make a concurrent call. do we really intend for these to not be reusable? i think the only problem we should protect against is using them for two concurrent calls as opposed to making them non-reusable. ning?
        Hide
        Bryan Duxbury added a comment -

        It's not just the error message. It's that the error you'd get if you tried to make two method calls with the same in-use TAsyncClient was concurrency related rather than state related.

        Show
        Bryan Duxbury added a comment - It's not just the error message. It's that the error you'd get if you tried to make two method calls with the same in-use TAsyncClient was concurrency related rather than state related.
        Hide
        Ning Liang added a comment -

        This looks like the fix we want, I think the name of the test is just a bit confusing. Maybe "testItRaisesExceptionOnConcurrentUsage"? The message thrown should be correct now - "Client is currently executing another method ..."

        Show
        Ning Liang added a comment - This looks like the fix we want, I think the name of the test is just a bit confusing. Maybe "testItRaisesExceptionOnConcurrentUsage"? The message thrown should be correct now - "Client is currently executing another method ..."
        Hide
        Bryan Duxbury added a comment -

        Fair enough. I'll commit the patch with that change in a second.

        Show
        Bryan Duxbury added a comment - Fair enough. I'll commit the patch with that change in a second.
        Hide
        Bryan Duxbury added a comment -

        I just committed this.

        Show
        Bryan Duxbury added a comment - I just committed this.
        Hide
        Zhenlei Cai added a comment -

        FYI I am not calling the same TAsyncClient from two threads, I am calling from only one thread it's just the calls are made very rapidly like 10000 calls per second, so one call starts before the other finishes.

        Show
        Zhenlei Cai added a comment - FYI I am not calling the same TAsyncClient from two threads, I am calling from only one thread it's just the calls are made very rapidly like 10000 calls per second, so one call starts before the other finishes.
        Hide
        Bryan Duxbury added a comment -

        With this patch, the error message will be improved, but it's still not OK to try to make another method call before the first one is complete.

        Show
        Bryan Duxbury added a comment - With this patch, the error message will be improved, but it's still not OK to try to make another method call before the first one is complete.
        Hide
        Eric Jensen added a comment -

        i've had to clarify this to a few people already. maybe we could improve the documentation somehow?

        Show
        Eric Jensen added a comment - i've had to clarify this to a few people already. maybe we could improve the documentation somehow?

          People

          • Assignee:
            Bryan Duxbury
            Reporter:
            Zhenlei Cai
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development