Page MenuHomePhabricator

Introduce Connection::ReadAll and fix AdbClient
ClosedPublic

Authored by labath on Apr 26 2016, 6:05 AM.

Details

Summary

AdbClient was attempting to handle the case where the socket input arrived in pieces, but it was
failing to handle the case where the connection was closed before that happened. In this case, it
would just spin in an infinite loop calling Connection::Read. (This was also the cause of the
spurious timeouts on the darwin->android buildbot. The exact cause of the premature EOF remains
to be investigated, but is likely a server bug.)

Since this wait-for-a-certain-number-of-bytes seems like a useful functionality to have, I am
moving it (with the infinite loop fixed) to the Connection class, and adding an
appropriate test for it.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 54997.Apr 26 2016, 6:05 AM
labath retitled this revision from to Introduce Connection::ReadAll and fix AdbClient.
labath updated this object.
labath added reviewers: clayborg, zturner.
labath added a subscriber: lldb-commits.
clayborg requested changes to this revision.Apr 26 2016, 9:43 AM
clayborg edited edge metadata.

The "timeout_usec" parameter to Connection::ReadAll() doesn't seem like it is doing what is intended. It uses this timeout for every read call which means the Connection::ReadAll() function could take much longer. If the timeout was say 1 second and every 0.9 seconds we get 1 byte, this function could take a very long time to complete. So that should be fixed. It also seems like ReadAll() could be done by adding an extra parameter to the Read() function like "bool keep_reading_until_timeout".

This revision now requires changes to proceed.Apr 26 2016, 9:43 AM
labath updated this revision to Diff 55397.Apr 28 2016, 3:37 AM
labath edited edge metadata.

Added proper handling of the timeout after retries, and moved the functionality into
Connection::Read. I have called the parameter read_full_buffer, as I think that better reflects
its intended use. I did not want to add a default value for it, since default values on virtual
functions can produce unexpected results. I've also had to increase the timeout in AdbClient,
since now it actually had a meaning (it seems that ReadAll was spinning in a loop also in case
the server was sending no data at all). The patch has grown bigger, but this is mostly due to
me refactoring unit tests to promote code reuse.

ovyalov edited edge metadata.Apr 28 2016, 10:47 AM

LGTM

source/Plugins/Platform/Android/AdbClient.cpp
37 ↗(On Diff #55397)

It might be useful to expose this timeout as a settings property to allow its customization for slow ADB connections.

clayborg accepted this revision.Apr 29 2016, 10:23 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Apr 29 2016, 10:23 AM
labath added inline comments.May 3 2016, 5:38 AM
source/Plugins/Platform/Android/AdbClient.cpp
37 ↗(On Diff #55397)

We can do it later if a need appears, but I don't see how making that a setting -- the value needs to be big enough to work under all circumstances, and in case of normal operation, it will not matter, as the function will return as soon as the data is ready.

This revision was automatically updated to reflect the committed changes.
labath added a comment.May 3 2016, 8:16 AM

This is going to be messier than I expected. I would need to introduce the same repeat-until-the-buffer-is-full loop into ConnectionGenericFileWindows, which I don't think is good for code reuse. I see two possibilities:

  • go back to the version with a separate function for this behavior (with the timeout problem fixed)
  • keep the fix completely within the AdbClient class.

Which one do you prefer?

I would prefer a fix in the ADB client only if I had to pick. I don't think anyone else will use this functionality and prefer to the keep the API as simple as possible for Connection subclasses.

labath added a comment.May 5 2016, 2:50 AM

Ok, I've put it into AdbClient for now. We can revisit it if we see a second user somewhere...