Thrift
  1. Thrift
  2. THRIFT-867

PHP accelerator module's output transport is incompatible with TFramedTransport

    Details

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

      Description

      I think we've figured out the cause of everyone's problems with THRIFT-837. The patch itself is fine; however, in fixing that bug, we've exposed the fact that PHPOutputTransport erroneously calls down to the underlying PHP transport's flush() method every time the internal 8k buffer is flushed. This is fine for the buffered transport, but unacceptable for the framed transport, which should only be flushed once per RPC call.

      It seems like what we need to do is separate the "internal" buffer flushes from the "external" transport flushes. If we do that, everything should work out fine.

      1. thrift-tframedtransport-867.patch
        0.5 kB
        Nicholas Telford
      2. thrift-867.diff
        0.4 kB
        Arya Goudarzi

        Activity

        Hide
        Arya Goudarzi added a comment -

        Actually, after investigating this more, I figured the problem is not with buffered being flushed once for FramedTransport, but that FramedTransport cares to exactly read the number of bytes as dictated to it by the first 4 bytes of the stream, otherwise it behaves incorrectly to the point that your operation will no succeed. The problem was that Accelerated Module, PHPOutputTransport class had a destructor calling out flush(). However the flush is called by the thrift_protocol_write_binary function inside the exception wrappers right where it needs to be after all the data is serialized into the stream, thus the extra flush inside destructor is not needed at all. This was not a problem for Buffered Transport because it reads everything it gets but this was causing problem for FramedTransport because it was causing an extra 0 (size of empty buffer after last flush) to be packed and placed at the end of stream making it confuse FramedTransport. So, attached please find the one liner patch to php_thrift_protocol.cpp. I tested this with Cassandra using FramedTransport up to 15Mb where I have it configured for max Frame size, and for variety of BufferTransport sizes. Feel free to do you own QA.

        Show
        Arya Goudarzi added a comment - Actually, after investigating this more, I figured the problem is not with buffered being flushed once for FramedTransport, but that FramedTransport cares to exactly read the number of bytes as dictated to it by the first 4 bytes of the stream, otherwise it behaves incorrectly to the point that your operation will no succeed. The problem was that Accelerated Module, PHPOutputTransport class had a destructor calling out flush(). However the flush is called by the thrift_protocol_write_binary function inside the exception wrappers right where it needs to be after all the data is serialized into the stream, thus the extra flush inside destructor is not needed at all. This was not a problem for Buffered Transport because it reads everything it gets but this was causing problem for FramedTransport because it was causing an extra 0 (size of empty buffer after last flush) to be packed and placed at the end of stream making it confuse FramedTransport. So, attached please find the one liner patch to php_thrift_protocol.cpp. I tested this with Cassandra using FramedTransport up to 15Mb where I have it configured for max Frame size, and for variety of BufferTransport sizes. Feel free to do you own QA.
        Hide
        Thomas Kho added a comment -

        Arya, would it be better to modify TFramedTransport not to write an empty frames on flush?

        Show
        Thomas Kho added a comment - Arya, would it be better to modify TFramedTransport not to write an empty frames on flush?
        Hide
        Arya Goudarzi added a comment -

        I did not try that and here is why:

        I think the transport, socket, and accelerated module have a degree of separation in which each handle specific task associated with themselves. For example TFramedTransport->flush(), does a write followed by flush and it formats its stream according to framed specification of size+buffer not knowing what was in its buffer. You are proposing to have a check in the flush() function which eliminates writing of empty sized buffers. I am not 100% sure if there is no use case in which writing of empty buffer is required, so I did not do it. But since the non accelerated module worked perfectly fine, it clearly says there is something not right with the other execution path. And there will be an extra if check for every flush which does not make sense when flush() after flush() is the problem. Thus, I stick with this patch unless it is causing some issue or I missed something I did not understand.

        Feel free to take a lead on this if you have a better solution.

        Show
        Arya Goudarzi added a comment - I did not try that and here is why: I think the transport, socket, and accelerated module have a degree of separation in which each handle specific task associated with themselves. For example TFramedTransport->flush(), does a write followed by flush and it formats its stream according to framed specification of size+buffer not knowing what was in its buffer. You are proposing to have a check in the flush() function which eliminates writing of empty sized buffers. I am not 100% sure if there is no use case in which writing of empty buffer is required, so I did not do it. But since the non accelerated module worked perfectly fine, it clearly says there is something not right with the other execution path. And there will be an extra if check for every flush which does not make sense when flush() after flush() is the problem. Thus, I stick with this patch unless it is causing some issue or I missed something I did not understand. Feel free to take a lead on this if you have a better solution.
        Hide
        Thomas Kho added a comment -

        Ah okay. Your explanation makes sense. Just want to note, though, that there's still a possibility for more than one transport flush to happen in sending a request since flush() is called whenever the buffer will overflow. I think your change only removes the sending of an empty frame. (Maybe there's a server bug in handling an empty frame?)

        Show
        Thomas Kho added a comment - Ah okay. Your explanation makes sense. Just want to note, though, that there's still a possibility for more than one transport flush to happen in sending a request since flush() is called whenever the buffer will overflow. I think your change only removes the sending of an empty frame. (Maybe there's a server bug in handling an empty frame?)
        Hide
        Tyler Hobbs added a comment -

        phpcassa (https://github.com/thobbs/phpcassa) ships with Arya's patch applied, and I haven't seen any problems with it using TFramedTransport.

        Show
        Tyler Hobbs added a comment - phpcassa ( https://github.com/thobbs/phpcassa ) ships with Arya's patch applied, and I haven't seen any problems with it using TFramedTransport.
        Hide
        Nicholas Telford added a comment -

        On Arya's points on the best place to fix this:

        • Arya's patch for the extension fixes a bug (namely that flush() gets called when it shouldn't). Whether or not this causes things to break is besides the point, it's unintended and should definitely be fixed.
        • Having TFramedTransport write empty frames on flush, whether you're using the extension or some other exotic protocol/transport, breaks servers that adhere to the FramedTransport format. While I've not seen a specification for this format, it seems implied that empty frames are not permitted. If this is the case, we should definitely have TFramedTransport::flush() check the frame isn't empty before writing it.

        My argument for the second point is one of coupling: a method should never rely on it's caller to call it in a particular order or validate it's state.

        Show
        Nicholas Telford added a comment - On Arya's points on the best place to fix this: Arya's patch for the extension fixes a bug (namely that flush() gets called when it shouldn't). Whether or not this causes things to break is besides the point, it's unintended and should definitely be fixed. Having TFramedTransport write empty frames on flush, whether you're using the extension or some other exotic protocol/transport, breaks servers that adhere to the FramedTransport format. While I've not seen a specification for this format, it seems implied that empty frames are not permitted. If this is the case, we should definitely have TFramedTransport::flush() check the frame isn't empty before writing it. My argument for the second point is one of coupling: a method should never rely on it's caller to call it in a particular order or validate it's state.
        Hide
        Bryan Duxbury added a comment -

        @Nicholas - should we commit this patch as-is then?

        Show
        Bryan Duxbury added a comment - @Nicholas - should we commit this patch as-is then?
        Hide
        Nicholas Telford added a comment -

        Yes definitely. My point is that we should also be patching TFramedTransport. I'll quickly roll a patch for that now.

        Show
        Nicholas Telford added a comment - Yes definitely. My point is that we should also be patching TFramedTransport. I'll quickly roll a patch for that now.
        Hide
        Nicholas Telford added a comment -

        Patch to make TFramedTransport::flush() only write a new frame to the underlying transport if there is some data to write.

        Show
        Nicholas Telford added a comment - Patch to make TFramedTransport::flush() only write a new frame to the underlying transport if there is some data to write.
        Hide
        Bryan Duxbury added a comment -

        I just tried to apply Arya's original patch, and it has at least one failing hunk. Nicholas, could I impose on you to see if you can massage it to apply cleanly and repost a combined patch for both changes?

        Show
        Bryan Duxbury added a comment - I just tried to apply Arya's original patch, and it has at least one failing hunk. Nicholas, could I impose on you to see if you can massage it to apply cleanly and repost a combined patch for both changes?
        Hide
        Nicholas Telford added a comment -

        Bryan, looking at the offending code in trunk, it's already been commented out. Seems like someone else has already applied a similar change.

        Show
        Nicholas Telford added a comment - Bryan, looking at the offending code in trunk, it's already been commented out. Seems like someone else has already applied a similar change.
        Show
        ruslan.usifov added a comment - see https://issues.apache.org/jira/browse/THRIFT-1067
        Hide
        Bryan Duxbury added a comment -

        I just committed this to trunk.

        Show
        Bryan Duxbury added a comment - I just committed this to trunk.

          People

          • Assignee:
            Unassigned
            Reporter:
            Bryan Duxbury
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development