Bug 169419 – Use TCP instead of FIFOs for Simulator/Device communication

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

We should TCP instead of FIFOs when communicating through stdin, stdout and stderr for WebKitTestRunnerApp and DumpRenderTree on Simulators and in the future, devices.  FIFO's will not work for on-device testing, so we should implement this communication in a way which will work for on-device testing.

[Radar WebKit Bug Importer]

<rdar://problem/30949615>

[Jonathan Bedard]

Created attachment 303929 [details]
Patch

[Jonathan Bedard]

Created attachment 303952 [details]
Patch

[Jonathan Bedard]

Created attachment 303983 [details]
Patch

[Daniel Bates]

Comment on attachment 303983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303983&action=review

> Tools/Scripts/webkitpy/port/ios.py:88
> +
> +        for i in xrange(self.child_processes()):
> +            self.device_for_worker_number(i).setup()
> +

Is there a reason we cannot create the sockets on demand?

> Tools/Scripts/webkitpy/xcode/device.py:68
> +        server = Device.UDID_TO_SOCKET[self.udid]
> +        if not server:
> +            starting_port = randint(Device.PORT_RANGE[0], Device.PORT_RANGE[1])
> +            upper_bound = Device.PORT_RANGE[1] - Device.PORT_RANGE[0]
> +
> +            for cnt in xrange(0, upper_bound):
> +                port = (starting_port + cnt) % upper_bound
> +                if port in Device.FORWARD_PORTS:
> +                    continue
> +                try:
> +                    server = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> +                    server.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
> +                    server.bind(('localhost', port))
> +                    break
> +                except socket.error, RuntimeError:
> +                    server = None
> +            Device.UDID_TO_SOCKET[self.udid] = server
> +
> +        if not Device.UDID_TO_SOCKET[self.udid]:
> +            raise RuntimeError('No open ports found')

It is unnecessary to implement logic to pick an available port as we can just ask the OS for one by binding to port 0: sever.bind(('localhost', 0)). Then we can use socket.getsockname() to determine the port the OS picked for us. There are various approaches that we can use to avoid the OS picking a port in FORWARD_PORTS - the ports that we want to reserve for the various servers we start up to run HTTP tests. (*) One approach is to create sockets on demand in SimulatorProcess and take advantage of the fact that we instantiate a SimulatorProcess after we start up all the web servers that we want to forward. This approach also has the side benefit that we do not need to have the hardcoded list FORWARD_PORTS, which is error prone. (I do not believe FORWARD_PORTS represents all the ports we need to forward. Among other things, it is missing the web socket server port, 9323.)

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:45
> +static FILE* originals[numHandles];

Is it necessary to save the original file handles?

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:64
> +    serverAddress.sin_port = htons(port);

The same port for each socket?

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:80
> +    for (unsigned i = 0; i < numHandles; ++i) {
> +        // Order matters!
> +        sockets[i] = socket(AF_INET, SOCK_STREAM, 0);
> +        if (sockets[i] < 0)
> +            abort();
> +        if (connect(sockets[i], (struct sockaddr *) &serverAddress, sizeof(serverAddress)) < 0)
> +            abort();
> +
> +        originals[i] = *referencesToHandles[i];
> +        fflush(originals[i]);
> +        if (i == stdinIndex)
> +            *referencesToHandles[i] = fdopen(sockets[i], "rb");
> +        else
> +            *referencesToHandles[i] = fdopen(sockets[i], "wb");
> +        setbuf(*referencesToHandles[i], nullptr);

I do not see the benefit of using a loop to setup the handles given that we only have three handles to setup and one of the handles requires special casing (standard in).

[Jonathan Bedard]

Comment on attachment 303983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303983&action=review

>> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:45
>> +static FILE* originals[numHandles];
> 
> Is it necessary to save the original file handles?

The reason this is done is so that when communication is closed, the process is returned to it's original state.

No, this is not needed for this to work, but without this, stdout, stderr and stdin will be left pointing to closed sockets when the communication is torn down, meaning writes or reads would fail.  This seems undesirable, especially since it's so easy to fix.

>> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:64
>> +    serverAddress.sin_port = htons(port);
> 
> The same port for each socket?

Yes!  That's the advantage of using one server socket per device.  All of the clients are connecting to the same server.  This reduces the number of arguments we have to pass in the environment and the number of ports we need to open.

>> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:80
>> +        setbuf(*referencesToHandles[i], nullptr);
> 
> I do not see the benefit of using a loop to setup the handles given that we only have three handles to setup and one of the handles requires special casing (standard in).

This reduces the amount of code if we are going to return file handles to their original state. (you seem to be against this on line 45).  Otherwise, yes, it would be more appropriate to setup the handles outside of this loop.

[Daniel Bates]

Comment on attachment 303983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303983&action=review

> Tools/Scripts/webkitpy/port/simulator_process.py:60
> +    # Python 2's implementation of makefile blocks on read if the
> +    # socket is open. Implement the small subset of commands we need.
> +    class ReadSocket(object):

Could we avoid the need for this class by making the socket non-blocking and using os.fdopen() with the socket's file descriptor (socket.fileno()) to get a File object?

[Jonathan Bedard]

Created attachment 304510 [details]
Patch

[Jonathan Bedard]

Created attachment 304658 [details]
Patch

[Daniel Bates]

Comment on attachment 304658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304658&action=review

> Tools/Scripts/webkitpy/port/simulator_process.py:25
> +import fcntl

This is unused.

> Tools/Scripts/webkitpy/port/simulator_process.py:95
> +        self._device.server().listen(3)

We need a comment here to explain why we are calling listen 3 times and describing the high-level design: We create one listening socket that will have three clients that represent the standard input, standard output, and standard error streams of the launched app.

[Jonathan Bedard]

Created attachment 304837 [details]
Patch

[Jonathan Bedard]

Created attachment 304928 [details]
Patch

[Jonathan Bedard]

Created attachment 305468 [details]
Patch

[Jonathan Bedard]

Created attachment 305601 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 305601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305601&action=review

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1240
> +    setupiOSLayoutTestCommunication();

"Set up" is two words, so the name should be setUpIOSLayoutTestCommunication (note upper case IOS too).

> Tools/Scripts/webkitpy/port/device.py:41
> +    def setup(self):

I don't think that set_up would be reasonable here, but maybe a more descriptive name would sidestep the naming problem.

> Tools/Scripts/webkitpy/port/device.py:45
> +            self.listening_socket.bind(('localhost', 0))

One issue that we had with localhost in the past is that it would flakily resolve to IPv6 loopback on some ancient OS versions, and cause strange failures. This is probably not a problem any more. Maybe it's good to be specific anyway.

> Tools/Scripts/webkitpy/port/ios_simulator.py:69
> +    #FIXME: Ports are recreated in each process.  This is a problem for IOSSimulatorPort, it means devices are not persistent

Please add a period at the end, and do not use French spacing.

It would be helpful to explain why having he devices not persistent is a problem.

> Tools/Scripts/webkitpy/port/simulator_process.py:58
> +    # Python 2's implementation of makefile does not return
> +    # a non-blocking file.

No need to wrap this line.

> Tools/Scripts/webkitpy/port/simulator_process.py:108
> +            # Order matters here!

A better comment would say something like "this order matches what is being sent in <function_name>".

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:47
> +    if (result < 0)
> +        abort();
> +    if (connect(result, (struct sockaddr *) &serverAddress, sizeof(serverAddress)) < 0)
> +        abort();

Would it be useful to print errors here? Or alternatively to crash. Otherwise, it will be hard to debug.

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:55
> +        abort();

Ditto. RELEASE_ASSERT?

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:57
> +    struct hostent* host = gethostbyname("localhost");

localhost or 127.0.0.1?

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:59
> +    memset((char *) &serverAddress, 0, sizeof(serverAddress));

Misplaced star.

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:67
> +    // Order matters!

:-|

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:78
> +void teardowniOSLayoutTestCommunication()

Two words, so "tearDownIOSLayoutTestCommunication" (note upper case IOS too).

> Tools/TestRunnerShared/iOSLayoutTestCommunication.cpp:88
> +void setupiOSLayoutTestCommunication() { }
> +void teardowniOSLayoutTestCommunication() { }

This doesn't seem needed.

[Jonathan Bedard]

Created attachment 305757 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 305757 [details]
Patch

Clearing flags on attachment: 305757

Committed r214553: <http://trac.webkit.org/changeset/214553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.