Bug 62180 – nrwt: fix http, websocket server startup, shutdown

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Dirk Pranke]

nrwt: fix http, websocket server startup, shutdown

[Dirk Pranke]

Created attachment 96185 [details]
Patch

[Dirk Pranke]

Created attachment 96352 [details]
Patch

[Dirk Pranke]

Created attachment 96359 [details]
ready for review

[Tony Chang]

Comment on attachment 96359 [details]
ready for review

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

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:48
> +    mappings = {}

It doesn't look like this dict should be shared by all instances that don't override it (e.g., it would be easy for a subclass to accidentally insert a value into this global dict).  I would just make it a member variable to make it more obvious that sub classes should be providing their own instance of it.

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:66
> +        except OSError, e:
> +            _log.warning('Failed to remove old %s log files' % self.name)

Should we just move this try/except into remove_log_files()?  It seems a bit weird that remove_stale_logs() needs to know what type of errors it can raise.

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:75
> +        _log.debug("%s started. Testing ports" % self.name)
> +        server_started = self._wait_for_action(self._is_server_running_on_all_ports)

It looks like we used to retry for a few seconds for the python server in case it was slow to start.  Should we do something similar here?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:90
> +        elif self.pid_file and self._filesystem.exists(self.pid_file):
> +            pid = int(self._filesystem.read_text_file(self.pid_file))

What cases would this be true?  Do we need to have a similar check in _is_server_running_on_all_ports()?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:107
> +    def prepare_config(self):
> +        pass

Should these interface methods be protected?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:144
> +            s = socket.socket()

Nit: s -> test_socket or something

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:147
> +                s.connect(('localhost', port))

This isn't quite the same check right?  The old code would make sure the server understood HTTP.  Maybe that's OK?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:160
> +            s = socket.socket()

Nit: s-> test_socket or something

[Dirk Pranke]

Comment on attachment 96359 [details]
ready for review

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

>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:48
>> +    mappings = {}
> 
> It doesn't look like this dict should be shared by all instances that don't override it (e.g., it would be easy for a subclass to accidentally insert a value into this global dict).  I would just make it a member variable to make it more obvious that sub classes should be providing their own instance of it.

OK.

>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:66
>> +            _log.warning('Failed to remove old %s log files' % self.name)
> 
> Should we just move this try/except into remove_log_files()?  It seems a bit weird that remove_stale_logs() needs to know what type of errors it can raise.

I understand the weirdness. I didn't want to repeat the code and I wanted to ensure the catch was done consistently, so this seem the better tradeoff. I suppose I could push the try/catch into the method and attempt to make it clear that the method should trap access denied and other such errors. Do you think that would be better (after I've explained the rationale).

>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:75
>> +        server_started = self._wait_for_action(self._is_server_running_on_all_ports)
> 
> It looks like we used to retry for a few seconds for the python server in case it was slow to start.  Should we do something similar here?

That's what _wait_for_action() does. I am removing the duplicate/different implementation in the python server code so that they all use the same consistent logic.

>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:90
>> +            pid = int(self._filesystem.read_text_file(self.pid_file))
> 
> What cases would this be true?  Do we need to have a similar check in _is_server_running_on_all_ports()?

This will eventually be true when this is being called by new-run-webkit-httpd. pid files aren't really useful as of this patch and just make things confusing, but I didn't want to totally delete the code just to put it back in later.

I'm not sure why you think we would need a similar check in _is_server_running, because that is purely doing a port check, it doesn't know *what* is running on those ports?

>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:107
>> +        pass
> 
> Should these interface methods be protected?

Sure.

>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:144
>> +            s = socket.socket()
> 
> Nit: s -> test_socket or something

OK.

>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:147
>> +                s.connect(('localhost', port))
> 
> This isn't quite the same check right?  The old code would make sure the server understood HTTP.  Maybe that's OK?

That is correct. However, we need to switch from urllib to sockets *because* urllib doesn't actually shut down the socket connection properly after connecting, and that causes us to get failures down the road when we try to restart the server. I figured doing a socket-level connect would be good enough, and (unlike urlopen), is protocol independent so I wouldn't have to recode things three or four different ways.

>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:160
>> +            s = socket.socket()
> 
> Nit: s-> test_socket or something

OK.

[Tony Chang]

Comment on attachment 96359 [details]
ready for review

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

>>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:66
>>> +            _log.warning('Failed to remove old %s log files' % self.name)
>> 
>> Should we just move this try/except into remove_log_files()?  It seems a bit weird that remove_stale_logs() needs to know what type of errors it can raise.
> 
> I understand the weirdness. I didn't want to repeat the code and I wanted to ensure the catch was done consistently, so this seem the better tradeoff. I suppose I could push the try/catch into the method and attempt to make it clear that the method should trap access denied and other such errors. Do you think that would be better (after I've explained the rationale).

Yeah, I was thinking we would push it into the method.  Seems fine to document that the method catches OSErrors.

>>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:75
>>> +        server_started = self._wait_for_action(self._is_server_running_on_all_ports)
>> 
>> It looks like we used to retry for a few seconds for the python server in case it was slow to start.  Should we do something similar here?
> 
> That's what _wait_for_action() does. I am removing the duplicate/different implementation in the python server code so that they all use the same consistent logic.

Ah, I see it now.  Sorry, wasn't in the diff.

>>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:90

>> 
>> What cases would this be true?  Do we need to have a similar check in _is_server_running_on_all_ports()?
> 
> This will eventually be true when this is being called by new-run-webkit-httpd. pid files aren't really useful as of this patch and just make things confusing, but I didn't want to totally delete the code just to put it back in later.
> 
> I'm not sure why you think we would need a similar check in _is_server_running, because that is purely doing a port check, it doesn't know *what* is running on those ports?

At the beginning of _is_server_running(), it calls:
  self._executive.check_running_pid(self._process.pid):

I was wondering if you needed to check self.pid_file as well as self._process.pid.

>>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:147
>>> +                s.connect(('localhost', port))
>> 
>> This isn't quite the same check right?  The old code would make sure the server understood HTTP.  Maybe that's OK?
> 
> That is correct. However, we need to switch from urllib to sockets *because* urllib doesn't actually shut down the socket connection properly after connecting, and that causes us to get failures down the road when we try to restart the server. I figured doing a socket-level connect would be good enough, and (unlike urlopen), is protocol independent so I wouldn't have to recode things three or four different ways.

This is probably fine.

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 96359 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96359&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:66
> >>> +            _log.warning('Failed to remove old %s log files' % self.name)
> >> 
> >> Should we just move this try/except into remove_log_files()?  It seems a bit weird that remove_stale_logs() needs to know what type of errors it can raise.
> > 
> > I understand the weirdness. I didn't want to repeat the code and I wanted to ensure the catch was done consistently, so this seem the better tradeoff. I suppose I could push the try/catch into the method and attempt to make it clear that the method should trap access denied and other such errors. Do you think that would be better (after I've explained the rationale).
> 
> Yeah, I was thinking we would push it into the method.  Seems fine to document that the method catches OSErrors.
> 

Okay.


> >> What cases would this be true?  Do we need to have a similar check in _is_server_running_on_all_ports()?
> > 
> > This will eventually be true when this is being called by new-run-webkit-httpd. pid files aren't really useful as of this patch and just make things confusing, but I didn't want to totally delete the code just to put it back in later.
> > 
> > I'm not sure why you think we would need a similar check in _is_server_running, because that is purely doing a port check, it doesn't know *what* is running on those ports?
> 
> At the beginning of _is_server_running(), it calls:
>   self._executive.check_running_pid(self._process.pid):
> 
> I was wondering if you needed to check self.pid_file as well as self._process.pid.
> 

Ah, no. _is_server_running() is only called during start(), after we spawn the process, and it's purpose is to ensure that the process did start up correctly. At that point the only pid you care about is the one in the process object. The latest pid hasn't even been written into a pidfile. The only reason we call check_running_pid is because the process may have exited between the spawn and this line. 

We could probably actually change this to self._process.poll(), so I'll do that because it's faster and clearer.

[Dirk Pranke]

Created attachment 96841 [details]
update w/ more review feedback

[Dirk Pranke]

Created attachment 96843 [details]
rebasing

[Dirk Pranke]

Created attachment 96844 [details]
rebasing again

[Tony Chang]

Comment on attachment 96844 [details]
rebasing again

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

> Tools/Scripts/webkitpy/layout_tests/port/http_server.py:192
> +        try:
> +            self._remove_log_files(self._output_dir, "access.log-")
> +        except OSError, e:
> +            _log.warning('Failed to remove old %s log files' % self._name)
> +        try:
> +            self._remove_log_files(self._output_dir, "error.log-")
> +        except OSError, e:
> +            _log.warning('Failed to remove old %s log files' % self._name)

Nit: A for loop would be a bit more succinct, but this is OK too.

[Dirk Pranke]

Created attachment 96995 [details]
rebase, update w/ last bits of feedback

[Dirk Pranke]

finally fixed in bug 62829.

*** This bug has been marked as a duplicate of bug 62829 ***