Bug 62829 – nrwt: attempt #3 at fixing server startup/shutdown

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: attempt #3 at fixing server startup/shutdown

[Dirk Pranke]

Created attachment 97510 [details]
Patch as landed in r88995 and r88999, later reverted (minus the changelog entries)

[Dirk Pranke]

Comment on attachment 97510 [details]
Patch as landed in r88995 and r88999, later reverted (minus the changelog entries)

This is the rolled-up version of the patches in bug 62251, 62256, bug 62180, and bug 62615 . This version didn't work; apache didn't get shut down correctly on either linux or Mac. I'm not sure why my tests didn't catch this yet.

[Dirk Pranke]

Created attachment 97914 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97914 [details]
Patch

Attachment 97914 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8910768

[WebKit Review Bot]

Created attachment 97916 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Pranke]

Created attachment 97917 [details]
lint, fix win error

[WebKit Review Bot]

Comment on attachment 97917 [details]
lint, fix win error

Attachment 97917 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8913767

[WebKit Review Bot]

Created attachment 97918 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Pranke]

Sheesh ... hopefully third time's the charm.

Okay, the previous attempt to fix the startup and shutdown of the http servers didn't work because apparently Apache lies to the caller about when it has started up or shut down.

For starters, it turns out the the process that you fork off to start apache completes immediately, and so you really need to look at the pid written to the apache pidfile, not the pid returned from the popen() call.

Next, you would normally expect to be able to do the equivalent of:

% apache $ARGS -k start
% cat $PIDFILE

to get the running pid. However, $PIDFILE may not have been written yet when the process completes, so you need to actually slow-spin waiting for the pid file (note also that if the pid file doesn't exist, the servers may not have been started). Similarly, when you call '-k stop', the pid file may not have been deleted yet, and the server not actually shut down.

This patch revises the code to address the above issues. I will also post an incremental diff from the version that landed previously and was reverted, because the overall patch is still quite large. 

I should probably follow up with the apache community at some point to figure out if things are supposed to work this way (although I can't imagine  why that would be correct), or if these are bugs. 

Note that the behavior reproduces on both Mac and Linux.

Now, you might ask why, given that there were tests written to make sure things were actually starting and stopping, why the tests were still passing. Welll, it appears that the python code is fast enough that it actually beats those races in such a way that the assertions about the servers being up or down are also "wrong", in that it would say the server was up, and then the server would shut down (or vice versa).

I have revised the tests to put in some random sleeps to catch this behavior, and added an integration test that starts and stops the servers in a loop just to be safe.

[Dirk Pranke]

Created attachment 97919 [details]
incremental diff from the version of the patch that was landed and then reverted.

[Dirk Pranke]

Note that the main change in this patch (apart from the logic fixes, obviously) is that self._process in the http_server is no longer particularly useful, so that stops being an instance variable, and instead there is a self._pid variable that tracks the pid that actually matters. In addition. self._spawn_process is now responsible for both launching the server and ensuring it is written to self._pid_file, since Apache and LigHTTPd/PyWebSocket accomplish these things differently.

[Tony Chang]

Comment on attachment 97919 [details]
incremental diff from the version of the patch that was landed and then reverted.

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

> Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:-57
> -        self._name = 'apache'

Why is this line being removed?

> Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:187
> +        if not self._wait_for_action(lambda: self._filesystem.exists(self._pid_file)):
> +            raise http_server_base.ServerError('Failed to start %s: no pid file found' % self._name)

Would it be possible to grab the pid after we do the socket connect check?  That would avoid having to wait twice.  I guess we would have to restructure the code quite a bit since that's not how the web socket server code works.  I guess this is fine since I don't have a better suggestion.

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:110
> +            _log.warning('Failed to stop %s: pid file contains %d, not %d' %
> +                         (self._name, actual_pid, self._pid))
> +            return

Should we stop some of the pids here?  If we hit this, it would be nice to not leave stray processes around.

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:149
> +    def _wait_for_action(self, action, wait_secs=2.0, sleep_secs=0.05):

2 seconds seems too short for starting apache.  How about something in the middle like 10s?

[Tony Chang]

Comment on attachment 97917 [details]
lint, fix win error

Do you know why the cr-linux bots failed?  It looks like some stray server processes.  That makes me hesitant to r+ this change.

[Dirk Pranke]

(In reply to comment #12)
> (From update of attachment 97919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97919&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:-57
> > -        self._name = 'apache'
> 
> Why is this line being removed?
> 

The version that was committed before had this instance variable being assigned twice.

> > Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:187
> > +        if not self._wait_for_action(lambda: self._filesystem.exists(self._pid_file)):
> > +            raise http_server_base.ServerError('Failed to start %s: no pid file found' % self._name)
> 
> Would it be possible to grab the pid after we do the socket connect check?  That would avoid having to wait twice.  I guess we would have to restructure the code quite a bit since that's not how the web socket server code works.  I guess this is fine since I don't have a better suggestion.
> 

Yeah, it's not easy to restructure this. However, both waits are very quick, in my experience, so I don't think it's worth worrying about.

> > Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:110
> > +            _log.warning('Failed to stop %s: pid file contains %d, not %d' %
> > +                         (self._name, actual_pid, self._pid))
> > +            return
> 
> Should we stop some of the pids here?  If we hit this, it would be nice to not leave stray processes around.

Hm. Good question. The only way this could happen is if the port object thinks it started something, but  the pid file points to something else, presumably because another port object killed the server and started a new one. In this case, normally, this port's server should already be dead, and we wouldn't want to kill the other one since we didn't start it.

I suppose we could kill self._pid directly just to be safe. I'll add that, and we can see if anything weird happens.

> 
> > Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:149
> > +    def _wait_for_action(self, action, wait_secs=2.0, sleep_secs=0.05):
> 
> 2 seconds seems too short for starting apache.  How about something in the middle like 10s?

Okay. I've never seen it take more than a second or so on my machine, but I suppose it's conceivable that there are much slower or more heavily loaded machines.

[Dirk Pranke]

(In reply to comment #13)
> (From update of attachment 97917 [details])
> Do you know why the cr-linux bots failed?  It looks like some stray server processes.  That makes me hesitant to r+ this change.

It seems possible or likely that the cr-linux bots already had stray servers running, which would be a fatal error in the new code. We may have to clean up the EWS bots just like I'm expecting to have to clean up all of the others in order to bring them into a known good state.

[Dirk Pranke]

Created attachment 98051 [details]
update w/ review feedback from tony

[Tony Chang]

Comment on attachment 98051 [details]
update w/ review feedback from tony

cr-linux passed this time.  weird.

[Dirk Pranke]

Comment on attachment 98051 [details]
update w/ review feedback from tony

Clearing flags on attachment: 98051

Committed r89400: <http://trac.webkit.org/changeset/89400>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

*** Bug 62256 has been marked as a duplicate of this bug. ***

[Dirk Pranke]

*** Bug 62180 has been marked as a duplicate of this bug. ***