Thrift
  1. Thrift
  2. THRIFT-1302

thrift: raise an exception if send() times out in

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8
    • Component/s: C++ - Library
    • Labels:
      None

      Description

      From 1f76284b5972ab01d6f6ac68f96024a8066a3b59 Mon Sep 17 00:00:00 2001
      From: Adam Simpkins <simpkins@fb.com>
      Date: Fri, 16 Apr 2010 17:43:21 +0000
      Subject: [PATCH 22/33] thrift: raise an exception if send() times out in
      TSocket::write()

      Summary:
      Previously, if send() timed out in TSocket::write(), it would sleep for
      50 microseconds and retry. This essentially made the timeout set with
      setSendTimeout() useless. Now it raises a TTransportException on
      timeout.

      TNonblockingServer does use TSocket with the fd manually put in
      non-blocking mode, and that could cause EWOULDBLOCK and EAGAIN to occur
      during normal operation. However, it only uses write_partial() and
      never write(), so it should be safe to have write() throw the exception.

      Test Plan:
      Used the client and server in thrift/tutorial/cpp/async/sort/ to test
      sending a large message to the server. I set a 100ms timeout in the
      client, and verified that it correctly times out now if the server
      process is stopped.

      I also ran [fb unittests] to try and verify that this doesn't negatively
      affect any other code.

      Revert Plan:
      OK


      lib/cpp/src/transport/TSocket.cpp | 7 ++++---
      1 files changed, 4 insertions, 3 deletions

        Activity

        Hide
        Nevo Hed added a comment -

        Questions to those involved in this patch -
        I may not have a full understanding of all the layers involved, but

        Wouldn't we want to "close()" the socket before we throw the exception?

        Just concerned that partial data may have been written already to the socket.

        I see that TSocket::write_partial does close on some errors, but not all. If we already wrote something - I am not sure how the client/server can keep in sync with a partial message and no tracking of the remaning bytes needed to complete it.

        Show
        Nevo Hed added a comment - Questions to those involved in this patch - I may not have a full understanding of all the layers involved, but Wouldn't we want to "close()" the socket before we throw the exception? Just concerned that partial data may have been written already to the socket. I see that TSocket::write_partial does close on some errors, but not all. If we already wrote something - I am not sure how the client/server can keep in sync with a partial message and no tracking of the remaning bytes needed to complete it.
        Hide
        Hudson added a comment -

        Integrated in Thrift #241 (See https://builds.apache.org/job/Thrift/241/)
        THRIFT-1302. cpp: raise an exception if send() times out in
        TSocket::write()

        Patch: Adam Simpkins

        bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1162923
        Files :

        • /thrift/trunk/lib/cpp/src/transport/TSocket.cpp
        Show
        Hudson added a comment - Integrated in Thrift #241 (See https://builds.apache.org/job/Thrift/241/ ) THRIFT-1302 . cpp: raise an exception if send() times out in TSocket::write() Patch: Adam Simpkins bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1162923 Files : /thrift/trunk/lib/cpp/src/transport/TSocket.cpp
        Hide
        Bryan Duxbury added a comment -

        Committed.

        Show
        Bryan Duxbury added a comment - Committed.

          People

          • Assignee:
            Dave Watson
            Reporter:
            Dave Watson
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development