Thrift
  1. Thrift
  2. THRIFT-517

TExceptions thrown by server result in cryptic error message on client - Tried to read 4 bytes, but only got 0 bytes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.6
    • Labels:
      None

      Description

      To reproduce:

      1. Define a simple Thrift service method with no custom exceptions.
      2. In server implementation of service method, throw a TException.
      3. Generated code in client forces you to do try/catch TException around service call, do this, and output stack trace,

      The exception thrown by the server then results in this rather cryptic error message in the client:

      26 22:32:44,931 INFO Caused by: org.apache.thrift.transport.TTransportException: Cannot read. Remote side has closed. Tried to read 4 bytes, but only got 0 bytes.
      26 22:32:44,931 INFO at org.apache.thrift.transport.TTransport.readAll(TTransport.java:86)
      26 22:32:44,931 INFO at org.apache.thrift.protocol.TBinaryProtocol.readAll(TBinaryProtocol.java:314)
      26 22:32:44,931 INFO at org.apache.thrift.protocol.TBinaryProtocol.readI32(TBinaryProtocol.java:262)
      26 22:32:44,931 INFO at org.apache.thrift.protocol.TBinaryProtocol.readMessageBegin(TBinaryProtocol.java:192)
      26 22:32:44,931 INFO at fm.last.catalogue.api.TCatalogueAPI$Client.recv_getTrackById(TCatalogueAPI.java:249)

      It would be better if the client got the actual TException sent by the server, or at the very least a better error message indicating that an unexpected error occurred on the server (akin to an HTTP 500 message that most web servers use).

        Activity

        Hide
        Bryan Duxbury added a comment -

        I committed a micro improvement that at least mentions that the server side might be to blame.

        Show
        Bryan Duxbury added a comment - I committed a micro improvement that at least mentions that the server side might be to blame.
        Hide
        Adrian Woodhead added a comment -

        I personally would like to get more detailed error messages on the client - in setups where thrift clients and servers are written by different teams of developers it helps each team know where the root cause of an exception is. When the server devs get bug reports it would also help a lot if there was more information than just "tried to read 4 bytes" as this is useless for figuring out what actually went wrong. We now end up writing try/catch throwable blocks in each and every thrift method so we can log the server exception and rethrow it as a checked exception which isn't exactly elegant but has to be done otherwise we have no idea where errors are occurring.

        If you are against it I would be happy to settle for at least receiving a less cryptic message - the equivalent of an HTTP 500 "something went wrong on the server".

        Show
        Adrian Woodhead added a comment - I personally would like to get more detailed error messages on the client - in setups where thrift clients and servers are written by different teams of developers it helps each team know where the root cause of an exception is. When the server devs get bug reports it would also help a lot if there was more information than just "tried to read 4 bytes" as this is useless for figuring out what actually went wrong. We now end up writing try/catch throwable blocks in each and every thrift method so we can log the server exception and rethrow it as a checked exception which isn't exactly elegant but has to be done otherwise we have no idea where errors are occurring. If you are against it I would be happy to settle for at least receiving a less cryptic message - the equivalent of an HTTP 500 "something went wrong on the server".
        Hide
        Bryan Duxbury added a comment -

        I am against this proposal. I think the current exception model is correct. I would be willing to improve the message of the clientside exception to make it more obvious that you should check your server's logs, if that would help.

        Show
        Bryan Duxbury added a comment - I am against this proposal. I think the current exception model is correct. I would be willing to improve the message of the clientside exception to make it more obvious that you should check your server's logs, if that would help.
        Hide
        David Reiss added a comment -

        I'd be curious to hear what others think of this proposal. Personally, I think it risks leaking information about the server's internal implementation that it might want to keep private.

        Show
        David Reiss added a comment - I'd be curious to hear what others think of this proposal. Personally, I think it risks leaking information about the server's internal implementation that it might want to keep private.
        Hide
        andrew zimmer added a comment -

        Why not just append the throwable's getMessage() or even toString() in the internal error text? I understand that this doesn't render the exception most elegantly, but I think by not including any details at all the client is pretty clueless. The debug process without any message to the client then results in confusion and requires that someone on the server side line up log timestamps with the operator on the client side.

        In a web app, it makes sense that one doesn't barf the stack trace to the user. But in this case I think you want to give as much detail as possible. Clients can still trap the general "unexpected" exception differently, but having a bit more detail in the logs would be hugely helpful for those of us on the server side who have a slew of code that throws RuntimeExceptions that shouldn't be turned into checked exceptions in Thrift.

        Here's how I hand patch the generated code:

        } catch (Throwable th) {
        LOGGER.error("Internal error processing [method name here]", th);
        TApplicationException x = new TApplicationException(TApplicationException.INTERNAL_ERROR, "Internal error processing [method name here: " + th.getMessage());

        It's a simple change and I think it would give the client a lot of useful information without breaking any exception models. It's just a log message, after all

        Show
        andrew zimmer added a comment - Why not just append the throwable's getMessage() or even toString() in the internal error text? I understand that this doesn't render the exception most elegantly, but I think by not including any details at all the client is pretty clueless. The debug process without any message to the client then results in confusion and requires that someone on the server side line up log timestamps with the operator on the client side. In a web app, it makes sense that one doesn't barf the stack trace to the user. But in this case I think you want to give as much detail as possible. Clients can still trap the general "unexpected" exception differently, but having a bit more detail in the logs would be hugely helpful for those of us on the server side who have a slew of code that throws RuntimeExceptions that shouldn't be turned into checked exceptions in Thrift. Here's how I hand patch the generated code: } catch (Throwable th) { LOGGER.error("Internal error processing [method name here] ", th); TApplicationException x = new TApplicationException(TApplicationException.INTERNAL_ERROR, "Internal error processing [method name here: " + th.getMessage()); It's a simple change and I think it would give the client a lot of useful information without breaking any exception models. It's just a log message, after all
        Hide
        Todd Lipcon added a comment -

        I always understood this as the difference between a TException and a TApplicationException of some sort. I think in some of the implementations, it will catch exceptions in the server code and serialize it back over the wire as a TApplicationException. I do something of this sort in Erlang, at least, if I recall correctly

        Show
        Todd Lipcon added a comment - I always understood this as the difference between a TException and a TApplicationException of some sort. I think in some of the implementations, it will catch exceptions in the server code and serialize it back over the wire as a TApplicationException. I do something of this sort in Erlang, at least, if I recall correctly
        Hide
        Bryan Duxbury added a comment -

        If you throw an anonymous TException on the server side, it's basically an "unexpected" exception. The way that the Thrift server code handles this is to log the exception and close the client connection. This is why your clientside gets a transport exception - the pipe closed on it unexpectedly.

        We could consider actually sending back the exception message from the TException you threw, but that is a little iffy. Without declaring that a method throws a certain exception type, how do we know what to encode it as on the wire? If we wanted to do this, we would probably have to extend the message-passing layer to include an undeclared exception message.

        I think that in principle, if you are going to throw an exception, declare it. Otherwise the semantics are just too confusing.

        Show
        Bryan Duxbury added a comment - If you throw an anonymous TException on the server side, it's basically an "unexpected" exception. The way that the Thrift server code handles this is to log the exception and close the client connection. This is why your clientside gets a transport exception - the pipe closed on it unexpectedly. We could consider actually sending back the exception message from the TException you threw, but that is a little iffy. Without declaring that a method throws a certain exception type, how do we know what to encode it as on the wire? If we wanted to do this, we would probably have to extend the message-passing layer to include an undeclared exception message. I think that in principle, if you are going to throw an exception, declare it. Otherwise the semantics are just too confusing.

          People

          • Assignee:
            Bryan Duxbury
            Reporter:
            Adrian Woodhead
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development