Bug 170144 - webkitpy: Standardize web-server port definitions
Summary: webkitpy: Standardize web-server port definitions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-27 15:12 PDT by Jonathan Bedard
Modified: 2017-03-29 13:07 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.22 KB, patch)
2017-03-27 15:18 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.53 KB, patch)
2017-03-28 09:16 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (12.16 KB, patch)
2017-03-29 09:30 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (12.20 KB, patch)
2017-03-29 12:01 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-03-27 15:12:27 PDT
Currently, web-servers receive their ports through a mixture of global constants and hard-coded port values.  Make web-servers use global constants to determine default port values and provide a function listing all used ports so that tools can know which ports need to be managed.
Comment 1 Radar WebKit Bug Importer 2017-03-27 15:12:59 PDT
<rdar://problem/31284026>
Comment 2 Jonathan Bedard 2017-03-27 15:18:50 PDT
Created attachment 305518 [details]
Patch
Comment 3 Daniel Bates 2017-03-27 16:52:52 PDT
Comment on attachment 305518 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:61
> +            self._mappings = [{'port': self.DEFAULT_PORTS[0]},
> +                              {'port': self.DEFAULT_PORTS[1]},
> +                              {'port': self.DEFAULT_PORTS[2], 'sslcert': True}]

It seems error prone to use an array to hold these ports numbers because the ordering of the array elements can affect the port used for the SSL port. I suggest we use named constants, say HTTP_SERVER_PORT, ALTERNATIVE_HTTP_SERVER_PORT and HTTPS_SERVER_PORT for 8000, 8080, and 8443, respectively.

> Tools/Scripts/webkitpy/layout_tests/servers/http_server.py:81
> +                {'port': self.DEFAULT_PORTS[2], 'docroot': self._webkit_tests,
>                   'sslcert': self._pem_file}])

Similarly, this is error prone because the ordering of self.DEFAULT_PORTS[2] can affect which port we chose to use as the SSL port.

On another note, I do not understand what the purpose of self._root is in this class? We have similar code below to setup the port mapping that was not updated and hence still hardcodes the port numbers 8000, 8080, and 8443. Did you intend to keep such code as-is?

> Tools/Scripts/webkitpy/port/base.py:942
> +    def webserver_ports(self):

webserver => web_server

Web server is two words.

> Tools/Scripts/webkitpy/port/base.py:992
> +                                use_tls=True, port=Port.WEBSOCKET_PORT, private_key=private_key_file, certificate=certificate_file)

Maybe Port.WEBSOCKET_SERVER_PORT would be more descriptive?

Would it make sense to move the named constants for the HTTP server ports to the base port class given that the base port class has a named constant for the WebSocket server port?
Comment 4 Jonathan Bedard 2017-03-28 09:12:16 PDT
Comment on attachment 305518 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/servers/http_server.py:81
>>                   'sslcert': self._pem_file}])
> 
> Similarly, this is error prone because the ordering of self.DEFAULT_PORTS[2] can affect which port we chose to use as the SSL port.
> 
> On another note, I do not understand what the purpose of self._root is in this class? We have similar code below to setup the port mapping that was not updated and hence still hardcodes the port numbers 8000, 8080, and 8443. Did you intend to keep such code as-is?

I didn't see the port mapping code bellow.

I'm not sure what the self._root is being used for.  It should definitely be adopting the constants, though.

>> Tools/Scripts/webkitpy/port/base.py:992
>> +                                use_tls=True, port=Port.WEBSOCKET_PORT, private_key=private_key_file, certificate=certificate_file)
> 
> Maybe Port.WEBSOCKET_SERVER_PORT would be more descriptive?
> 
> Would it make sense to move the named constants for the HTTP server ports to the base port class given that the base port class has a named constant for the WebSocket server port?

Actually, we should do the opposite.  PyWebSocket should own it's own constants.  In fact, it already does, they are just made private.
Comment 5 Jonathan Bedard 2017-03-28 09:16:07 PDT
Created attachment 305596 [details]
Patch
Comment 6 Daniel Bates 2017-03-29 07:52:11 PDT
Comment on attachment 305596 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:104
> +        result = []
> +        for dict in self._mappings:
> +            result.append(dict['port'])
> +        return result

We can use list comprehension to simplify this to:

return [dict['port'] for dict in self._mappings]

> Tools/Scripts/webkitpy/port/base.py:940
> +    def web_server_ports(self):

Maybe a better name for this function would be ports_to_forward()?

> Tools/Scripts/webkitpy/port/base.py:955
> +        ports = []
> +        port_from_options = self.get_option('http_port')
> +        if port_from_options is not None:
> +            ports.append(int(port_from_options))
> +        else:
> +            ports.extend([
> +                http_server_base.HttpServerBase.HTTP_SERVER_PORT,
> +                http_server_base.HttpServerBase.ALTERNATIVE_HTTP_SERVER_PORT,
> +                http_server_base.HttpServerBase.HTTPS_SERVER_PORT,
> +            ])
> +        ports.extend([
> +            websocket_server.PyWebSocket.DEFAULT_WS_PORT,
> +            websocket_server.PyWebSocket.DEFAULT_WSS_PORT,
> +        ])
> +

This does not seem like the correct approach to use because it is fragile and duplicates logic for determining the ports that will be opened. The PyWebSocket class and HttpServerBase class or derived classes of it are the most knowledgeable about the ports they actually chose to open. I suggest that we expose a method on PyWebSocket and HttpServerBase called ports_to_forward() (I am open to name suggestions) that returns an array of the ports that were opened. In HttpServerBase, ports_to_forward() should raise a not implemented exception and the derived classes Lighttpd and LayoutTestApacheHttpd should override the base implementation. Then we can implement this function as:

ports = []
if self._http_server:
    ports.extend(self._http_server.ports_to_forward())
if self._websocket_server:
    ports.extend(self._websocket_server.ports_to_forward())
if self._websocket_secure_server:
    ports.extend(self._websocket_secure_server.ports_to_forward())
if self._web_platform_test_server:
    ports.extend(self._web_platform_test_server.ports_to_forward())
return ports
Comment 7 Jonathan Bedard 2017-03-29 09:30:22 PDT
Created attachment 305749 [details]
Patch
Comment 8 Daniel Bates 2017-03-29 10:55:59 PDT
Comment on attachment 305749 [details]
Patch

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

OK

> Tools/ChangeLog:9
> +        Default web-server ports should be declared in global variables.

Is this description up-to-date?

> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:-48
> -_DEFAULT_WSS_PORT = 9323

I am assuming that this was unused before his patch as I did not see any caller updated in this patch.
Comment 9 Jonathan Bedard 2017-03-29 12:01:33 PDT
Created attachment 305767 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2017-03-29 13:07:46 PDT
Comment on attachment 305767 [details]
Patch for landing

Clearing flags on attachment: 305767

Committed r214558: <http://trac.webkit.org/changeset/214558>
Comment 11 WebKit Commit Bot 2017-03-29 13:07:49 PDT
All reviewed patches have been landed.  Closing bug.