• 
      

    Fix data truncation, blocking I/O, and performance issues in rbssh.

    Review Request #12574 — Created Sept. 2, 2022 and submitted

    Information

    Review Board
    release-4.0.x

    Reviewers

    rbssh has been in use for many, many years, and has worked in the cases
    that people most often use it for (Git, Subversion, etc.). However, a
    new integration that remote-controls a server over SSH has unveiled a
    problem where we can stop processing the channel before we finish
    handling all stdout/stderr content.

    This will happen if the other end sends a large payload of data
    (extending a buffer beyond our read limit of 4096) without blocking, and
    then closes the stream with an EOF.

    Paramiko will see the EOF and then mark the channel as "closed". This is
    sort of a lie. "closed" does not mean "stop reading", as we've
    interpreted it, but rather "no more data will be come in." There's a
    difference there, as the buffers may still contain content.

    This was one issue. Another is that we'd try to output data and,
    sometimes this would result in a Resource Not Available error. This
    depends on the version of Python and how the data is being consumed
    (e.g., outputting to iTerm on macOS using Python 2.7 can trigger this,
    but redirecting to a file does not).

    These have been solved by doing two things:

    1. We no longer assume we're done reading if the channel is marked as
      closed. Instead, we only close if we've received an exit code, we did
      not receive new data, and we don't have any data left in the buffers.

    2. stdout/stderr are now non-blocking, like stdin. This ensures data
      gets sent immediately to the caller, and forces us to handle writes
      accordingly (which could be an issue if a caller were to set our
      blocking flags for those streams).

    With these fixed, we're also able to increase the buffer sizes to
    improve performance.

    For stdout/stderr data, we now read up to 16KB at a time (it will not
    wait for a full 16KB, it'll just read up to that).

    For stdin, we also read up to 16KB, instead of 1 byte. This is far more
    efficient. We use the same blocking-aware logic we now have for working
    with stdout/stderr streams.

    Unit tests pass.

    Tested the new integration that occasionally triggered the buffer
    truncation issue. With this fix, the issue no longer occurred across
    many dozens of repeated attempts.

    Ran many tests using Python 2.7 and various 3.x versions where I ran
    commands on multiple remote machines, checking for blocking I/O issues
    and truncated data. These included using cat to transfer data to a
    remote server and to transfer from a remote server, verifying hashes.

    Tested performance and verified a pretty substantial (several seconds)
    decrease in time when transferring 100MB+ files.

    Summary ID
    Fix data truncation, blocking I/O, and performance issues in rbssh.
    rbssh has been in use for many, many years, and has worked in the cases that people most often use it for (Git, Subversion, etc.). However, a new integration that remote-controls a server over SSH has unveiled a problem where we can stop processing the channel before we finish handling all stdout/stderr content. This will happen if the other end sends a large payload of data (extending a buffer beyond our read limit of 4096) without blocking, and then closes the stream with an EOF. Paramiko will see the EOF and then mark the channel as "closed". This is sort of a lie. "closed" does not mean "stop reading", as we've interpreted it, but rather "no more data will be come in." There's a difference there, as the buffers may still contain content. This was one issue. Another is that we'd try to output data and, sometimes this would result in a Resource Not Available error. This depends on the version of Python and how the data is being consumed (e.g., outputting to iTerm on macOS using Python 2.7 can trigger this, but redirecting to a file does not). These have been solved by doing two things: 1. We no longer assume we're done reading if the channel is marked as closed. Instead, we only close if we've received an exit code, we did not receive new data, and we don't have any data left in the buffers. 2. stdout/stderr are now non-blocking, like stdin. This ensures data gets sent immediately to the caller, and forces us to handle writes accordingly (which could be an issue if a caller were to set our blocking flags for those streams). With these fixed, we're also able to increase the buffer sizes to improve performance. For stdout/stderr data, we now read up to 16KB at a time (it will not wait for a full 16KB, it'll just read up to that). For stdin, we also read up to 16KB, instead of 1 byte. This is far more efficient. We use the same blocking-aware logic we now have for working with stdout/stderr streams.
    73df92c5b545a66c2440dcb9dcb4c1c93100dad8
    Description From Last Updated

    Typo: ouput -> output

    maubinmaubin

    Oops another typo: avilable -> available

    maubinmaubin

    Should we have a Version Added: for this and for write_output?

    maubinmaubin

    Should we have a Version Changed: here for the new channel arg?

    maubinmaubin
    maubin
    1. 
        
    2. reviewboard/cmdline/rbssh.py (Diff revision 1)
       
       
      Show all issues

      Typo: ouput -> output

    3. 
        
    chipx86
    maubin
    1. 
        
    2. reviewboard/cmdline/rbssh.py (Diff revisions 1 - 2)
       
       
      Show all issues

      Oops another typo: avilable -> available

    3. reviewboard/cmdline/rbssh.py (Diff revision 2)
       
       
      Show all issues

      Should we have a Version Added: for this and for write_output?

    4. reviewboard/cmdline/rbssh.py (Diff revision 2)
       
       
       
       
       
       
       
       
       
      Show all issues

      Should we have a Version Changed: here for the new channel arg?

    5. 
        
    chipx86
    david
    1. Ship It!
    2. 
        
    maubin
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-4.0.x (7321fbd)