Bug 62180 - nrwt: fix http, websocket server startup, shutdown
Summary: nrwt: fix http, websocket server startup, shutdown
Status: RESOLVED DUPLICATE of bug 62829
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 62256
Blocks: 59993 62297 62615
  Show dependency treegraph
 
Reported: 2011-06-06 19:52 PDT by Dirk Pranke
Modified: 2011-06-22 11:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch (21.54 KB, patch)
2011-06-06 19:54 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (19.73 KB, patch)
2011-06-07 18:55 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
ready for review (19.70 KB, patch)
2011-06-07 19:32 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ more review feedback (22.93 KB, patch)
2011-06-10 20:40 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
rebasing (41.15 KB, patch)
2011-06-10 20:51 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
rebasing again (22.11 KB, patch)
2011-06-10 20:52 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
rebase, update w/ last bits of feedback (23.43 KB, patch)
2011-06-13 13:12 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-06-06 19:52:24 PDT
nrwt: fix http, websocket server startup, shutdown
Comment 1 Dirk Pranke 2011-06-06 19:54:53 PDT
Created attachment 96185 [details]
Patch
Comment 2 Dirk Pranke 2011-06-07 18:55:45 PDT
Created attachment 96352 [details]
Patch
Comment 3 Dirk Pranke 2011-06-07 19:32:54 PDT
Created attachment 96359 [details]
ready for review
Comment 4 Tony Chang 2011-06-10 15:32:03 PDT
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
Comment 5 Dirk Pranke 2011-06-10 15:55:16 PDT
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.
Comment 6 Tony Chang 2011-06-10 16:05:37 PDT
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.
Comment 7 Dirk Pranke 2011-06-10 19:01:50 PDT
(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.
Comment 8 Dirk Pranke 2011-06-10 20:40:02 PDT
Created attachment 96841 [details]
update w/ more review feedback
Comment 9 Dirk Pranke 2011-06-10 20:51:03 PDT
Created attachment 96843 [details]
rebasing
Comment 10 Dirk Pranke 2011-06-10 20:52:17 PDT
Created attachment 96844 [details]
rebasing again
Comment 11 Tony Chang 2011-06-13 10:02:07 PDT
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.
Comment 12 Dirk Pranke 2011-06-13 13:12:01 PDT
Created attachment 96995 [details]
rebase, update w/ last bits of feedback
Comment 13 Dirk Pranke 2011-06-22 11:05:03 PDT
finally fixed in bug 62829.

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