792262 - DeviceManagerSUT does not detect closed connections properly

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Mark Côté [:mcote]]

The _doCmds() loop does not properly detect when a connection is shut down/closed. The standard way to do this is if the socket is readable but recv() returns 0 bytes. An exception will not normally be thrown in this instance.

Technically recv() can return 0 bytes if the other end has just shut down the write half of the connection, but that is good enough for us, since the agent side of the connection shouldn't ever be in a read-only mode.

Comment 1

[Mark Côté [:mcote]]

Attachment: close when socket readable but recv() returns 0 bytes

This patch raises an AgentError if select indicates that the socket is readable but recv() returns nothing. This is the standard way to indicate that the other end has closed at least half of the connection.

I removed the loop guard because it was doing precisely this, but only after 1000 tries.

sut.py passes, as do all the sut_tests, and I got a completely green try run on this patch (along with other recent unmerged patches): https://tbpl.mozilla.org/?tree=Try&rev=e4cf72096f5a

Comment 2

[Mark Côté [:mcote]]

There is also a unit test for bug 783569 in here as well that works properly now.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 662562 [details] [diff] [review]
close when socket readable but recv() returns 0 bytes

Review of attachment 662562 [details] [diff] [review]:
-----------------------------------------------------------------

nice.

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ +240,4 @@
>              timer += select_timeout
>              if timer > timeout:
>                raise AgentError("Automation Error: Timeout in command %s" % cmd['cmd'], fatal=True)
> +          except socket.error, err: 

nit: whitespace here

::: mozdevice/tests/sut.py
@@ +65,5 @@
> +                              ("cwd", None),
> +                              ("cd /mnt/sdcard/tests", ""),
> +                              ("cwd", "/mnt/sdcard/tests"),
> +                              ("ver", "SUTAgentAndroid Version XX")])
> +        

nit: whitespace here

@@ +122,1 @@
>          

nit: whitespace here

Comment 4

[William Lachance (:wlach)]

Comment on attachment 662562 [details] [diff] [review]
close when socket readable but recv() returns 0 bytes

Looks good, minus a few style nits.

>diff --git a/mozdevice/mozdevice/devicemanagerSUT.py b/mozdevice/mozdevice/devicemanagerSUT.py
>index e717f4c..85341c6 100644
>--- a/mozdevice/mozdevice/devicemanagerSUT.py
>+++ b/mozdevice/mozdevice/devicemanagerSUT.py

>-        while found == False and (loopguard < recvGuard):
>+        while found == False:
>+          socket_closed = False

Should be socketClosed to be consistent with style in rest of file.

Also, while we're mucking around with this code, let's rename found to a more descriptive "foundPrompt". And let's say "while not foundPrompt" too.

>diff --git a/mozdevice/tests/sut.py b/mozdevice/tests/sut.py
>index ecbcfa8..348f14c 100644
>--- a/mozdevice/tests/sut.py
>+++ b/mozdevice/tests/sut.py
>@@ -13,15 +13,21 @@ import time
> class BasicTest(unittest.TestCase):
> 
>     def _serve_thread(self):
>-        conn, addr = self._sock.accept()
>-        conn.send("$>\x00")
>+        conn = None
>         while self.commands:
>+            if not conn:
>+                conn, addr = self._sock.accept()
>+                conn.send("$>\x00")
>             (command, response) = self.commands.pop(0)
>             data = conn.recv(1024).strip()
>             self.assertEqual(data, command)
>             # send response and prompt separately to test for bug 789496
>             if response is None:
>-                time.sleep(3)
>+                conn.shutdown(socket.SHUT_RDWR)
>+                conn.close()
>+                conn = None
>+            elif type(response) is int:
>+                time.sleep(response)
>             else:
>                 conn.send("%s\n" % response)
>                 conn.send("$>\x00")

As we discussed on irc, this is becoming a bit of a mess. I think we need to abstract out a better mock agent here, but that can wait for another bug. Just flagging this as an issue.

Comment 5

[Mark Côté [:mcote]]

Pushed with nits addressed. I also added a FIXME note to the tests so we (hopefully) remember to eventually fix up the mock agent.

https://github.com/mozilla/mozbase/commit/a57cdb2306deb4b6a5b9f6233c8e2b1542c908df