multi-patch: Running multiple instances of run-webkit-tests is not possible on the same machine

[Andras Becsi]

Running more than one instances of run-webkit-tests on the same machine is not possible due to more reasons.
The main issues are related to httpd port number, but there are also tests which use a hardcoded symlink of the LayoutTests directory in /tmp to load local pages, which is absolutely not a multi-platform friendly thing to do.

So the main issues are: 
- Hardcoded port numbers (run-webkit-tests --port does not work correctly)
Testcases (html, javascript and expected files) involved:
Hardcoded 8000 (http):
   total files involved: ~600
Hardcoded 8443 (https):
   total files involved: ~34
Hardcoded 8880 (websocket):
   total files involved: ~30

(The above sets may not be disjunct.)

- Symlinked and hardcoded LayoutTests dir to /tmp/LayoutTests may cause problems on this tests if testing multiple revisions:
LayoutTests/fast/dom/frame-loading-via-document-write.html
LayoutTests/fast/loader/local-CSS-from-local.html
LayoutTests/fast/loader/local-JavaScript-from-local.html
LayoutTests/fast/loader/local-iFrame-source-from-local.html
LayoutTests/fast/loader/local-image-from-local.html
LayoutTests/fast/loader/user-style-sheet-resource-load-callbacks.html
LayoutTests/http/tests/misc/image-blocked-src-change.html
LayoutTests/http/tests/misc/image-blocked-src-no-change.html
LayoutTests/http/tests/security/frame-loading-via-document-write.html
LayoutTests/http/tests/security/local-CSS-from-remote.html
LayoutTests/http/tests/security/local-JavaScript-from-remote.html
LayoutTests/http/tests/security/local-iFrame-from-remote.html
LayoutTests/http/tests/security/local-image-from-remote.html
LayoutTests/http/tests/security/local-user-CSS-from-remote.html
LayoutTests/http/tests/security/local-video-poster-from-remote.html
LayoutTests/http/tests/security/local-video-source-from-remote.html
LayoutTests/http/tests/security/local-video-src-from-remote.html

- Hardcoded pidfile path of the httpd daemon (not platform independent)

There are more possible ways of fixing these issues, so it would be nice to get some input and ideas on how to fix these.

[Andras Becsi]

(In reply to comment #0)
First of all we need to change the pidfile path to use File::spec->tmpdir(). 
One way would be to implement setter and getter functions to layoutTestController to be able to get the preconfigured port number and LayoutTests dir path in javascript (via environmental variable for example) and update the related files, so no need for /tmp/LayoutTests. In this case we need to update all the related tests to use these functions.

Another way would be to use a locking mechanism on httpd, run the http tests at the end of the testing, and let other instances wait until the running finishes. This is not a very robust way to do (and not parallel), a crashed testing session could introduce a deadlock, but only the ~17 symlink related tests would need an update and layoutTestController one function to get the LayoutTests dirpath.

[Andras Becsi]

Created attachment 45802 [details]
1st step: change hardcoded /tmp to File::Spec->tmpdir()

Change hardcoded /tmp in httpd pid file path to File::Spec->tmpdir() and remove the pidfile if closing the httpd.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45802 [details] without any errors.

[Darin Adler]

Comment on attachment 45802 [details]
1st step: change hardcoded /tmp to File::Spec->tmpdir()

> +my $httpdPidDir = File::Spec->tmpdir()."/WebKit";

We normally put spaces around operators like the "." here.

> -    mkdir "/tmp/WebKit";
> +    mkdir "$httpdPidDir";

It doesn't make sense to keep the quote marks here.

> +    if (-f "$httpdPidFile") {

Or here.

> -    kill 15, `cat /tmp/WebKit/httpd.pid` if -f "/tmp/WebKit/httpd.pid";
> +    kill 15, `cat "$httpdPidFile"` if -f "$httpdPidFile";

Or here on the argument to -f.

> +    rmtree "$httpdPidDir";

Or here.

I am surprised the old code did not remove the pid directory.

I think it would be more elegant to call unlink on the file and then rmdir on the directory instead of using rmtree here.

I'm going to say review- even though my comments are only minor style things. It would be nice to make it a little cleaner.

[Alexey Proskuryakov]

Why would one want to run multiple instances of run-webkit-tests on the same machine? Bug 10906 talks about parallelizing the script, but it's not clear what the goal is here.

[Csaba Osztrogonác]

(In reply to comment #5)
> Why would one want to run multiple instances of run-webkit-tests on the same
> machine? Bug 10906 talks about parallelizing the script, but it's not clear
> what the goal is here.

Because one machine can be a powerful multiprocessor and multicore server which can solve out several developer and run several buildbots, try bots, etc. This feature could help a lot for us.

[Alexey Proskuryakov]

Supporting parallelism inside run-webkit-tests, and also being able to run several instances of the script sounds like overkill. Did you consider running multiple buildbots in virtual machines?

In any case, making http and websocket tests port-agnostic is not practical. Most of them are not, and those that are got significantly more complicated because of that. Putting the burden of writing port-agnostic tests on developers is a cost we'd have to keep paying in the future, and thus undesirable.

I was actually planning to remove the half-baked ability to configure port numbers in run-webkit-tests.

There are more difficulties with running multiple instances of the script than mentioned in this bug - please see bug 10906 for discussion.

[Eric Seidel (no email)]

I see a real need for this bug.  We've hit this several times at google with people trying to share Macs for running the LayoutTests.  I could have sworn I filed a bug about this already actually.  The first time I hit this is when Julie Parent was SSHing into my machine to do development, and we found that only one of us could run the layout tests.

I see this as unrelated to the multi-threaded run-layout-tests.  We'll get multi-threaded run-layout-tests when run_webkit_tests.py lands later this week.  That's just about being able to run the layout tests faster.  This bug is about making it possible for more than one user account on a machine to do WebKIt development, or for more than one bot to run on a machine w/o using a VM.

[Eric Seidel (no email)]

I do however agree with Alexey that there are more issues with running multiple copies of run-webkit-tests than mentioned in this bug.  I also agree with him that making those tests port-agnostic is probably not practical.  However there might be solutions inside DRT which can be used to redirect the port numbers instead of fixing the tests themselves.

I see this bug as being a useful feature, but not one critical to my day-to-day work.  I will however rejoice if it's ever finished! :)

[Andras Becsi]

(In reply to comment #7)
> Supporting parallelism inside run-webkit-tests, and also being able to run
> several instances of the script sounds like overkill. Did you consider running
> multiple buildbots in virtual machines?
We use virtual machines now, but using them means significant loss of horsepower and is a huge wastage of energy, time and the capabilities of our server, so, on the contrary: using virtual machines only for running the tests is overkill, as I see it.

> In any case, making http and websocket tests port-agnostic is not practical.
> Most of them are not, and those that are got significantly more complicated
> because of that. Putting the burden of writing port-agnostic tests on
> developers is a cost we'd have to keep paying in the future, and thus
> undesirable.
I do not think that changing related tests to use and expect a preconfigured port number (which would be fixed per-buildbot in our usecase) would make writing tests significantly complicated.

> There are more difficulties with running multiple instances of the script than
> mentioned in this bug - please see bug 10906 for discussion.
On our development servers we are running multiple instances of run-webkit-tests --no-http parallel without any problems except of very rare issues related to the /tmp/LayoutTests symlink.

Bug 10906 seems quiet dead and is dealing with a much more complicated issue, which wouldn't help us to maximalize the efficiency of our hardware, and help other developers who want to test on their shared developer hardware resource more effectively.

[Andras Becsi]

(In reply to comment #9)
> I do however agree with Alexey that there are more issues with running multiple
> copies of run-webkit-tests than mentioned in this bug.  I also agree with him
> that making those tests port-agnostic is probably not practical.  However there
> might be solutions inside DRT which can be used to redirect the port numbers
> instead of fixing the tests themselves.

We'll try to find a solution which does need the least changes to the layout tests. 

Thanks for the input.

[Eric Seidel (no email)]

This genre of bug gets in the way of running things like the Mac Early Warning Systems (mac-ews), because right now we need one machine per bot instance since only one run-layout-test instance can run on any machine at once.  Since it's against the Mac OS X license to run Mac OS X inside a VM (at least that's my understanding) there is no VM workaround for Mac. :(

[Csaba Osztrogonác]

(In reply to comment #2)
> Created an attachment (id=45802) [details]
> 1st step: change hardcoded /tmp to File::Spec->tmpdir()
> 
> Change hardcoded /tmp in httpd pid file path to File::Spec->tmpdir() and remove
> the pidfile if closing the httpd.

In sub openHTTPDIfNeeded() you should override apache PID and scoreboard files defined in apache config files with -c option. eg.

my @args = {
...
    "-c", "PidFile $httpdPidFile",
    "-c", "ScoreBoardFile $httpdPidDir/httpd.scoreboard",
...

[Andras Becsi]

Created attachment 45883 [details]
1st step 2nd try

Change hardcoded /tmp in run-webkit-tests to File::Spec->tmpdir() and remove the httpd's pidfile directory if httpd terminated.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45883 [details] without any errors.

[Darin Adler]

Comment on attachment 45883 [details]
1st step 2nd try

> +        my $oldPid = `cat $httpdPidFile`;

Doing it this way assumes there are no spaces in tmpdir. I'm not sure that will always be true. To make this work in all cases quoting since you are passing this path to the shell.

> +    die "Timed out waiting for httpd to terminate" unless $retryCount;

Why is this a good idea?

> +    rmdir $httpdPidDir;

How can this rmdir work on a non-empty directory? What deletes the file?

[Andras Becsi]

(In reply to comment #16)
> (From update of attachment 45883 [details])
> > +        my $oldPid = `cat $httpdPidFile`;
> 
> Doing it this way assumes there are no spaces in tmpdir. I'm not sure that will
> always be true. To make this work in all cases quoting since you are passing
> this path to the shell.
Right, the quotes would make it more robust. I'll fix that.
> 
> > +    die "Timed out waiting for httpd to terminate" unless $retryCount;
> 
> Why is this a good idea?
> 
> > +    rmdir $httpdPidDir;
> 
> How can this rmdir work on a non-empty directory? What deletes the file?
If httpd terminates it deletes the httpd.pid and httpd.scoreboard files form $httpdPidDir, that is why we need a while loop after sending kill 15 to the process. After successfull termination the directory is empty. I've put the die there to indicate that someting went wrong, because after sending kill 15 and sleeping 20 secs the process still didn't terminate.

[Darin Adler]

(In reply to comment #17)
> I've put the die
> there to indicate that someting went wrong, because after sending kill 15 and
> sleeping 20 secs the process still didn't terminate.

But is aborting the rest of the test run helpful?

[Andras Becsi]

Created attachment 45893 [details]
1st step: another try

Thanks Darin, you are right, aborting there would cause that the websocket server wouldn't be stopped either, so it is better to just print a message to stderr.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45893 [details] without any errors.

[Darin Adler]

Comment on attachment 45893 [details]
1st step: another try

> +        my $oldPid = `cat "$httpdPidFile"`;

Just for the record, this works for spaces and single quotes and other metacharacters, but not for dollar signs or double quote marks. It's easy to read a number out of a file without using backticks or invoking the shell; that would be more robust. Not important for this patch.

> +    while ((0 != kill 0, $httpdPid) && $retryCount) {

I find this while expression really confusing. Especially the ", $httpPid" part of it. And I also can't tell what value the "0 !=" adds in the expression "0 != kill 0".

The code will now return, leaving $isHttpdOpen set to 1, if the pid file doesn't exist. I'm not sure that's a good change in behavior.

r=me as is, but you could consider my comments above too even though I don't think any are showstoppers

[Gabor Loki]

Comment on attachment 45893 [details]
1st step: another try

Clearing flags on attachment: 45893

Committed r52853: <http://trac.webkit.org/changeset/52853>

[Gabor Loki]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

To hit the target we need more patches, we shouldn't close the bug prematurely.

[Andras Becsi]

(In reply to comment #21)
> (From update of attachment 45893 [details])
> > +        my $oldPid = `cat "$httpdPidFile"`;
> 
> Just for the record, this works for spaces and single quotes and other
> metacharacters, but not for dollar signs or double quote marks. It's easy to
> read a number out of a file without using backticks or invoking the shell; that
> would be more robust. Not important for this patch.
> 
> > +    while ((0 != kill 0, $httpdPid) && $retryCount) {
> 
> I find this while expression really confusing. Especially the ", $httpPid" part
> of it. And I also can't tell what value the "0 !=" adds in the expression "0 !=
> kill 0".
> 
> The code will now return, leaving $isHttpdOpen set to 1, if the pid file
> doesn't exist. I'm not sure that's a good change in behavior.
> 
> r=me as is, but you could consider my comments above too even though I don't
> think any are showstoppers
Thanks Darin. The mentioned issues have to be addressed in a follow up, because the backticks and the while loop are used this way multiple times in the script and I wanted to retain the consistency as far as possible.
I'm planning to address the isHttpdOpen problem too.

[Eric Seidel (no email)]

It is believed this caused bug 33256.  I'm attempting a rollout now.

[Andras Becsi]

Created attachment 46043 [details]
Reworked r52853 after r52876 rolled it out.

Chomp the pid variable before passing it to kill and read the contents of the pidfile in a platform independent manner.
The missing chomp seemed to cause problems on Leopard.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46043 [details] without any errors.

[Csaba Osztrogonác]

(In reply to comment #27)
> Created an attachment (id=46043) [details]

Sending        WebKitTools/ChangeLog
Sending        WebKitTools/Scripts/run-webkit-tests
Transmitting file data ..
Committed revision 52917.

Landed, and bots are still green. :)

[Csaba Osztrogonác]

Comment on attachment 46043 [details]
Reworked r52853 after r52876 rolled it out.

landed, flags cleared.

[Andras Becsi]

Created attachment 46052 [details]
2nd step: run-webkit-tests --http-as-last option

Implement run-webkit-tests --http-as-last option to be able to run the http tests after all other tests.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46052 [details] without any errors.

[Andras Becsi]

Further explanation:

Redirecting of ports in DRT is not possible, so we need a locking mechanism for httpd. We need to minimalize the time where httpd is locked by a run-webkit-tests instance and because closeHTTPD() is only called at the end of the testing, we need to run http tests after all other tests, so the httpd gets only locked if it is really used.

[Alexey Proskuryakov]

Comment on attachment 46052 [details]
2nd step: run-webkit-tests --http-as-last option

This can be common behavior, no need to introduce an option.

What about WebSocket tests? Do you plan to do the same change for those?

[Darin Adler]

(In reply to comment #34)
> This can be common behavior, no need to introduce an option.

I agree.

[Andras Becsi]

Created attachment 46063 [details]
2nd: step again

I made it default behaviour to run http and websocket tests after all other tests. Thx Alexey for the advice.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46063 [details] without any errors.

[Darin Adler]

Comment on attachment 46063 [details]
2nd: step again

Another way to do this would be to make a new comparison function that takes this into account after calling pathcmp. But this seems OK.

[Csaba Osztrogonác]

(In reply to comment #29)
> (In reply to comment #27)
> > Created an attachment (id=46043) [details] [details]
> 
> Sending        WebKitTools/ChangeLog
> Sending        WebKitTools/Scripts/run-webkit-tests
> Transmitting file data ..
> Committed revision 52917.

Unfortunately we had to rollout it again by http://trac.webkit.org/changeset/52939 (Because it broke Leopards) Later I will submit here detailed errors and logs.

[Csaba Osztrogonác]

(In reply to comment #38)
> (From update of attachment 46063 [details])
> Another way to do this would be to make a new comparison function that takes
> this into account after calling pathcmp. But this seems OK.

I'll try to land it later (at night) to avoid the angry of WebKit folks if something might goes wrong. :)

[Csaba Osztrogonác]

Comment on attachment 46063 [details]
2nd: step again

Sending        WebKitTools/ChangeLog
Sending        WebKitTools/Scripts/run-webkit-tests
Transmitting file data ..
Committed revision 52976. 

http://trac.webkit.org/changeset/52976

[Eric Seidel (no email)]

(In reply to comment #41)
> (From update of attachment 46063 [details])
> Sending        WebKitTools/ChangeLog
> Sending        WebKitTools/Scripts/run-webkit-tests
> Transmitting file data ..
> Committed revision 52976. 
> 
> http://trac.webkit.org/changeset/52976

Looks like due to bad layout tests, this change broke all mac bots, and indirectly the Snow Leopard bot and Windows Bots.

The Mac bots were diagnosed and fixed as bug 32294.
The Snow Leopard failure is bug 33372
The Windows failure is bug 33374

I think we may have to roll this out, even though it's not really the fault of this change.  The currently awake developers do not have enough platforms at our disposal to diagnose the failing tests.

[Andras Becsi]

(In reply to comment #42)
> (In reply to comment #41)
> > (From update of attachment 46063 [details] [details])
> > Sending        WebKitTools/ChangeLog
> > Sending        WebKitTools/Scripts/run-webkit-tests
> > Transmitting file data ..
> > Committed revision 52976. 
> > 
> > http://trac.webkit.org/changeset/52976
> 
> Looks like due to bad layout tests, this change broke all mac bots, and
> indirectly the Snow Leopard bot and Windows Bots.
> 
> The Mac bots were diagnosed and fixed as bug 32294.
> The Snow Leopard failure is bug 33372
> The Windows failure is bug 33374
> 
> I think we may have to roll this out, even though it's not really the fault of
> this change.  The currently awake developers do not have enough platforms at
> our disposal to diagnose the failing tests.

If it is necessary roll it out, then we have to depend on those bugs. Or I can change the behaviour (running http and websocket tests as last) to be an option as I originally planed, and later make it common.
I really think it would be a better idea to skip the failing tests on the failing bots.

[Eric Seidel (no email)]

Actually the best solution is probably to just check in new results for these tests for windows and snow-leopard.  Skipping is also OK, but the webkit community seems to be pretty against using the Skipped list.

[Eric Seidel (no email)]

The run-http-tests at the end patch has made us lose the ability to control test order.  http tests are always at the end now.

run-webkit-tests http/sometest fast/another_test

used to work to control order.  But now it doesn't.

The ability to have run-webkit-tests run tests in the same order that you pass them on the command line was useful to me at times.  I don't think w should lose that.

[Andras Becsi]

(In reply to comment #45)
> The run-http-tests at the end patch has made us lose the ability to control
> test order.  http tests are always at the end now.
> 
> run-webkit-tests http/sometest fast/another_test
> 
> used to work to control order.  But now it doesn't.
> 
> The ability to have run-webkit-tests run tests in the same order that you pass
> them on the command line was useful to me at times.  I don't think w should
> lose that.
Actually that was one cause why I originally wanted to make it to an option, and seeing the tumble I caused I am more and more of the opinion that changing it to a run-webkit-tests switch is the only way to pour oil on troubled waters.

[Eric Seidel (no email)]

OK.  Please post a patch to turn it into an option.

We also need to revert the following changes:
http://trac.webkit.org/changeset/52980
http://trac.webkit.org/changeset/52990
http://trac.webkit.org/changeset/52992

If one of you posted a patch to do all of that, that would be great.  Otherwise I can just roll out all 4 changes.

[Alexey Proskuryakov]

There is still no need to make an option. The right way to handle this is to only change test order if it wasn't explicitly specified (i.e. if run-webkit-tests wasn't passed any directories as arguments).

[Csaba Osztrogonác]

(In reply to comment #45)
> The run-http-tests at the end patch has made us lose the ability to control
> test order.  http tests are always at the end now.
> 
> run-webkit-tests http/sometest fast/another_test
> 
> used to work to control order.  But now it doesn't.
> 
> The ability to have run-webkit-tests run tests in the same order that you pass
> them on the command line was useful to me at times.  I don't think w should
> lose that.

Yes, we shouldn't lose this option. I'm convinced that we use the original
version of this patch with command line option. It can make everybody happy. We need possibility to run https tests after others, AP need enabled tests, everybody need possibility to chose run order, etc.

[Andras Becsi]

Created attachment 46379 [details]
1st step again

Store Apache PID in a variable and override Apache settings to put the
PID file into the directory where run-webkit-tests expects it to be.
If shut down properly run-webkit-tests should delete the PID directory.
This is needed in preparation to be able to set a per-instance tmp dir to running run-webkit-tests.

[Andras Becsi]

(In reply to comment #50)
> Created an attachment (id=46379) [details]
> 1st step again
> 
> Store Apache PID in a variable and override Apache settings to put the
> PID file into the directory where run-webkit-tests expects it to be.
> If shut down properly run-webkit-tests should delete the PID directory.
> This is needed in preparation to be able to set a per-instance tmp dir to
> running run-webkit-tests.

I forgot to add that this time I tested it on a Leopard machine.

[Alexey Proskuryakov]

You are only patching run-webkit-tests. I'm concerned that this may interact poorly with run-webkit-httd, did you test this combination?

[Eric Seidel (no email)]

IMO, run-webkit-tests should be re-written to use run-webkit-httpd.  It's silly to have copy-paste code between the two. :(

[Eric Seidel (no email)]

Also (and this should not block this work in any way), you should be aware that Dirk is working on giving webkit.org a python version of run_webkit_tests which Chromium has been using for some time.  It has some wizbang features like being able to run all the tests in < 2m on my machine due to its use of multiple DRT instances. :)  Ojan has likewise re-written run-webkit-httpd in python in the Chromium repository.  My eventually goal is for us to finish up-streaming the chromium architecture, and then for webkit to make intelligent decisions about which parts to adopt and which parts to chuck.  This should not block your work, but you should be aware of it.  It may already be possible for instance to run more than one copy of run_webkit_tests.py concurrently on the same machine.

[Darin Adler]

The Python version should probably take over the name run-webkit-tests rather than having a similar but subtly different name.

[Andras Becsi]

(In reply to comment #53)
> IMO, run-webkit-tests should be re-written to use run-webkit-httpd.  It's silly
> to have copy-paste code between the two. :(

I'll refactor run-iexploder-tests, run-webkit-httpd and run-webkit-tests to use a common function to open Apache.

[Andras Becsi]

(In reply to comment #54)
> Also (and this should not block this work in any way), you should be aware that
> Dirk is working on giving webkit.org a python version of run_webkit_tests which
> Chromium has been using for some time.  It has some wizbang features like being
> able to run all the tests in < 2m on my machine due to its use of multiple DRT
> instances. :)  Ojan has likewise re-written run-webkit-httpd in python in the
> Chromium repository.  My eventually goal is for us to finish up-streaming the
> chromium architecture, and then for webkit to make intelligent decisions about
> which parts to adopt and which parts to chuck.  This should not block your
> work, but you should be aware of it.  It may already be possible for instance
> to run more than one copy of run_webkit_tests.py concurrently on the same
> machine.

Thanks Eric, I was aware of the python version of run-webkit-tests from chromium. 
I suppose we can switch to the python version as soon as possible if it gets into mainline, but until then we need this feature to be implemented in the perl script.

[Andras Becsi]

Created attachment 46575 [details]
Extract Apache handling to httpd.pm module

Extract Apache handling to httpd.pm module and use the provided functionality in scripts where Apache is needed.
The module httpd.pm stores the PID of the current Apache daemon in a variable and cleans up the PID directory after Apache properly shut down.
Catching INT and TERM signals allows the scrips to shut down Apache properly even if the testing was interrupted.

The next step will be enabling per-instance temp dirs for run-webkit-tests, so there will be no conflict with leftower files in /tmp and the undeleted symlink /tmp/LayoutTests, which is needed for some tests. This however requires changes in some 20 tests which hardcode /tmp in their javascript code to get the tmp dir path from LayoutTestController.
After enabling the setting of a custom path for local storage and icon database (located now in .local), the last step will be the --wait-for-httpd switch for run-webkit-tests which uses a locking mechanism to ensure that only one tester at a time runs the http tests.

[Chris Jerdonek]

(In reply to comment #58)
> Created an attachment (id=46575) [details]
> Extract Apache handling to httpd.pm module

One suggestion: can you put new Perl "library" files under WebKitTools/Scripts/webkitperl?  Thanks.

[Andras Becsi]

(In reply to comment #59)
> (In reply to comment #58)
> > Created an attachment (id=46575) [details] [details]
> > Extract Apache handling to httpd.pm module
> 
> One suggestion: can you put new Perl "library" files under
> WebKitTools/Scripts/webkitperl?  Thanks.

In this case I suggest moving WebKitTools/Scripts/SpacingHeuristics.pm, WebKitTools/Scripts/VCSUtils.pm, WebKitTools/Scripts/webkitdirs.pm also to WebKitTools/Scripts/webkitperl in a follow-up.

[Andras Becsi]

Created attachment 46580 [details]
Moved httpd.pm to webkitperl directory

[Chris Jerdonek]

(In reply to comment #60)
> (In reply to comment #59)
> > (In reply to comment #58)
> > > Created an attachment (id=46575) [details] [details] [details]
> > > Extract Apache handling to httpd.pm module
> > 
> > One suggestion: can you put new Perl "library" files under
> > WebKitTools/Scripts/webkitperl?  Thanks.
> 
> In this case I suggest moving WebKitTools/Scripts/SpacingHeuristics.pm,
> WebKitTools/Scripts/VCSUtils.pm, WebKitTools/Scripts/webkitdirs.pm also to
> WebKitTools/Scripts/webkitperl in a follow-up.

Agreed -- thanks.  I don't know if this is applicable to your current work with httpd.pm, but another thing to notice is that we can do Perl unit tests now.  You could follow the pattern for VCSUtils and establish an httpd_unittest folder.  test-webkitperl (and test-webkit-scripts) automatically includes any tests in folders with names of the form *_unittest in that directory.

[Andras Becsi]

Created attachment 46673 [details]
Updated changelog to correctly show added module path

Yesterday I forgot to update the changelog after moving the httpd.pm to webkitperl directory.
I tested all the scripts on a Leopard machine to see if they also work correctly on Mac, but I also set the cq? flag now to be sure that Eric's cq-bot also checks my changes if somebody reviews.

Chris, unfortunately I have no time for writing unit tests for this module and see absolutelly no need for that, because this is not a frequently changing code as the VCS stuff.

[Alexey Proskuryakov]

Comment on attachment 46673 [details]
Updated changelog to correctly show added module path

+        * Scripts/webkitperl/httpd.pm: Added.

I'm not sure what the rules for placing Perl modules are. Some are in WebKitTools/Scripts, others in WebKitTools/Scripts/webkitperl.

+my $iExploderLogDir = "/tmp/iExploderLogs";

We don't normally use abbreviations if we can avoid them - iExploderLogDirectory would be better.

+configAndOpenHTTPDIfNeeded();

Ditto.

+    system "WebKitTools/Scripts/run-safari", "-NSOpen", "$iExploderLogDir/redirect.html";

If it's named "log directory", then it's not OK to keep other things there.

-my $testDirectory = getcwd() . "/LayoutTests";
+my $testDirectory = catfile(getcwd(), "LayoutTests");

Perl documentation says that File::Spec subroutines should not be called directly, but rather as class methods: File::Spec->catfile('a','b');. I'm not sure what to make out of this.

+my $testResultsDirectory = catfile($tmpDir, "layout-test-results");

Ditto. This is repeated several more times in this patch.

+    my @httpdArgs = (
+    "-f", "$httpdConfig",
+    "-C", "DocumentRoot \"$documentRoot\"",

Seems like these should be indented.

+sub setHTTPDStandalone

It's not really clear what this function does, it needs a better name. Maybe something related to "waitUntilQuit"?

+$isHttpdOpen = closeHTTPD();

closeHTTPD() doesn't explicitly return any value. Does this code do what it's supposed to do in Perl?

+    # Setup a link to where the js test templates are stored, use -c so that mod_alias will already be laoded.

This is just moved code, but: "loaded".

+my @defaultArgs = getDefaultConfigForTestDir($testDirectory);
+@args = (@defaultArgs, @args);

This code looks fragile. Can there be some check for conflicting arguments?

This generally looks same, and is a great step in the right direction.

[Alexey Proskuryakov]

> This generally looks same

Sane!!!

[Chris Jerdonek]

(In reply to comment #64)
> (From update of attachment 46673 [details])
> +        * Scripts/webkitperl/httpd.pm: Added.
> 
> I'm not sure what the rules for placing Perl modules are. Some are in
> WebKitTools/Scripts, others in WebKitTools/Scripts/webkitperl.

There is no official rule, but the idea going forward was to keep the user-callable scripts in WebKitTools/Scripts, and to put new supporting Perl files in WebKitTools/Scripts/webkitperl.  The Python files are organized similarly (with webkitpy in place of webkitperl).

The supporting Perl files already in WebKitTools/Scripts (like VCSUtils.pm) can be moved into webkitperl at a later date.

[Andras Becsi]

Created attachment 47012 [details]
Updated patch

[Andras Becsi]

(In reply to comment #64)
Thanks Alexey for the suggestions.

> I'm not sure what the rules for placing Perl modules are. Some are in
> WebKitTools/Scripts, others in WebKitTools/Scripts/webkitperl.
As Chris explained, we plan to move all the perl modules to WebKitTools/Scripts/webkitperl in a follow-up, which is not a subject of this bug.

> We don't normally use abbreviations if we can avoid them -
> iExploderLogDirectory would be better.
> If it's named "log directory", then it's not OK to keep other things there.
Renamed

> Perl documentation says that File::Spec subroutines should not be called
> directly, but rather as class methods: File::Spec->catfile('a','b');. I'm
> not sure what to make out of this.
I changed it to File::Spec->catfile, but there are many more places where catfile is used without File::Spec in run-webkit-tests.

> Seems like these should be indented.
Done

> It's not really clear what this function does, it needs a better name. Maybe
> something related to "waitUntilQuit"?
Renamed to waitForUserInterrupt()

> +$isHttpdOpen = closeHTTPD();
> 
> closeHTTPD() doesn't explicitly return any value. Does this code do what
> it's supposed to do in Perl?
closeHTTPD() returns 0 if the closing of Apache succeeded it returns 1 if for some reason the termination timed out. We need to maintain the state on Apache, because to die in closeHTTPD() might cause some trouble in run-webkit-tests.

> +my @defaultArgs = getDefaultConfigForTestDir($testDirectory);
> +@args = (@defaultArgs, @args);
> 
> This code looks fragile. Can there be some check for conflicting arguments?
I do not really think there are conflicting arguments, and the argument list is a list of strings (not an associative array), which can't be checked effectively. Afaik, Apache uses the last specified options if there is duplication (for example -f) and different other configuration options can be set with -c and -C. I think checking whether the given options are correct is not the scope of this module, it should be checked by the programmer.

[Alexey Proskuryakov]

Comment on attachment 47012 [details]
Updated patch

> closeHTTPD() returns 0 if the closing of Apache succeeded it returns 1 if for
> some reason the termination timed out.

Indeed - I was looking at an old version of this function when I made this comment.

It's unfortunate to have a function that returns 0 on success, and 1 on failure. Please consider changing this - even having to negate the result each time seems like a low price for consistent return values.

+sub waitForUserInterrupt
+{
+    $standalone = 1;
+}

This function doesn't wait, it only sets a bit - so it should be called e.g. "setShouldWaitForUserInterrupt". Also, $standalone should be renamed, too.

r=me, but at least the latter naming issue should be fixed before landing. You may want to submit an updated patch for commit-queue, or someone can make the changes locally, and land this one.

[Andras Becsi]

Created attachment 47051 [details]
update2

Thanks Alexey. I've made both changes.

[Csaba Osztrogonác]

Comment on attachment 47051 [details]
update2

CQ won't work without r+ on this patch, I will land it manually. Alexey r+-ed your previous patch, but ask you to fix these thing before landing. You did it, so it is ready for landing.

[Csaba Osztrogonác]

Comment on attachment 47051 [details]
update2

Landed in http://trac.webkit.org/changeset/53559

[Andras Becsi]

Created attachment 47202 [details]
Implement a locking and scheduling mechanism for http testing sessions

With this we are able to run multiple instances of run-webkit-tests parallel.                        
Setting WEBKIT_WAIT_FOR_HTTPD=1 environment variable before run-webkit-tests enables the feature.
There are issues with precisely simultaneous LocalStorage tests but a patch is in progress.

[David Levin]

Removed [] in summary per webkit-dev email.

[Andras Becsi]

(In reply to comment #21)
> Created an attachment (id=47202) [details]
> Implement a locking and scheduling mechanism for http testing sessions
> 
> With this we are able to run multiple instances of run-webkit-tests parallel.   
> Setting WEBKIT_WAIT_FOR_HTTPD=1 environment variable before run-webkit-tests
> enables the feature.
> There are issues with precisely simultaneous LocalStorage tests but a patch is
> in progress.

Darin, could you please take a look at my latest patch? We need this in to be to start our new buildbots.

Thanks.

[Darin Adler]

(In reply to comment #75)
> Darin, could you please take a look at my latest patch? We need this in to be
> to start our new buildbots.

I might get a chance soon, but please note that I am only one of the 55 WebKit reviewers.

[Eric Seidel (no email)]

(In reply to comment #71)
> (From update of attachment 47051 [details])
> CQ won't work without r+ on this patch, I will land it manually. Alexey r+-ed
> your previous patch, but ask you to fix these thing before landing. You did it,
> so it is ready for landing.

cq+ should work w/o an r+.  it just won't update the reviewer for you.  We recently added this feature to support "land-safely"

[Andras Becsi]

Created attachment 47565 [details]
Rebase and update the changelog and fix a comment typo

I humbly ask somebody for a review, or at least constructive suggestions on how to improve my solution.

Thank you very much.

[Andras Becsi]

Created attachment 47618 [details]
Style fixes and simlifications as discussed on IRC

[Darin Adler]

Comment on attachment 47618 [details]
Style fixes and simlifications as discussed on IRC

> +my $shouldWaitForHTTPD = ($ENV{"WEBKIT_WAIT_FOR_HTTPD"})?1:0;

Extra parentheses here. And usually we would use spaces around operators, like the "?" and the ":".

And there's no real reason to include the "? 1 : 0" because the sites using the variable will evaluate it in a boolean context just as this expression would.

> +if (getWaitTime() > 0.5) {

Seems strange to call getWaitTime() three times instead of just once. How did you choose "0.5" as the cut-off?

> +my $exclusiveLock = File::Spec->catfile($tmpDir, "WebKit.lock");

The other variables with filenames in them have "file" or "path" at the end of their names.

> -            die "Timed out waiting for httpd to quit" unless $retryCount;
> +            if (!$retryCount) {
> +                cleanup();
> +                die "Timed out waiting for httpd to quit" unless $retryCount;
> +            }

Should remove the "unless $retryCount".

> +sub cleanup

The verb "clean up" is two words, so this should be "cleanUp" rather than "cleanup". YOu did not add it, though.

> +    my ($lockFile) = @_;
> +    my $number = -1;
> +    $number = substr($lockFile, length($httpdLockPrefix)) if $lockFile;
> +    return $number;

I would write this differently.

    return -1 if $lockFile;
    return substr($lockFile, length($httpdLockPrefix));

> +    opendir (TMPDIR, $tmpDir);

We normally don’t use spaces before parentheses like this. Also we normally put "or die" after this kind of function call.

I have to go do something else now, and can't finish reviewing, but I wanted to post my comments on the first part of the patch.

[Andras Becsi]

> I have to go do something else now, and can't finish reviewing, but I wanted to
> post my comments on the first part of the patch.
Thank you Darin. I'll address these. The

[Andras Becsi]

Created attachment 47742 [details]
updated patch

Suggested issues corrected and furder simplified the helper methods.

The bug is getting very big and confusing now whith its many patches and discussions, this scares off reviewers, so we will file other bugs to depend on for this master bug to address the remaining minor issues.

[Andras Becsi]

Created attachment 47768 [details]
Rebased patch after r54084

[Brian Weinstein]

I've been seeing some issues with the buildbots on Windows (where once one http test times out, they all start timing out), and I believe it's r53559 that caused it, but I don't have any evidence behind this.

Did this test change the way run-webkit-tests recovers from a timeout in http tests?

Any feedback on whether or not this could be causing that issue would be great.

Example stdio:

http://build.webkit.org/builders/Windows%20Release%20%28Tests%29/builds/8571/steps/layout-test/logs/stdio

Once http/tests/security/aboutBlank/xss-DENIED-navigate-opener-document-write.html -> timed out, then every other test timed out. I'm worried about how run-webkit-tests is recovering from an http timeout after r53559.

Thanks

[Andras Becsi]

(In reply to comment #84)
> I've been seeing some issues with the buildbots on Windows (where once one http
> test times out, they all start timing out), and I believe it's r53559 that
> caused it, but I don't have any evidence behind this.
> 
> Did this test change the way run-webkit-tests recovers from a timeout in http
> tests?
> 
> Any feedback on whether or not this could be causing that issue would be great.

r53559 did not change the behaviour at timeout and if the timeout would be related to apache's start or stop (which was refactored), there should be red stderr messages where the timeout occured in the log you showed.

The logs of Apache are very interesting however in this case:
http://build.webkit.org/results/Windows%20Release%20%28Tests%29/r54106%20%288571%29/access_log.txt
http://build.webkit.org/results/Windows%20Release%20%28Tests%29/r54106%20%288571%29/error_log.txt

Especially the error-log, are many lines like:
[Sat Jan 30 19:54:12 2010] [notice] child pid 2396 exit signal User defined signal 2 (31)
which start before the timeouting xss-DENIED-navigate-opener-document-write.html in the access_log,
127.0.0.1 - - [30/Jan/2010:19:55:34 -0800] "GET /security/aboutBlank/xss-DENIED-navigate-opener-document-write.html HTTP/1.1" 200 3540

Then there is no access log of any other http tests after the timeout and no error messages in stdout which would indicate DRT could not access Apache. This might suggest some strange DRT bug or something similar.
At the end of the error_log there is:

[Sat Jan 30 19:55:51 2010] [notice] child pid 1976 exit signal User defined signal 2 (31)
[Sat Jan 30 21:57:58 2010] [warn] child process 284 still did not exit, sending a SIGTERM
[Sat Jan 30 21:58:02 2010] [error] child process 284 still did not exit, sending a SIGKILL
[Sat Jan 30 21:58:03 2010] [notice] caught SIGTERM, shutting down

What happened in the one hour between 19:55:51 and 21:57:58, where there is no error_log entry at all?
This sigterm at the end is the script initiated kill 15 at the end of the session, but what are these strange sigquit ("User defined signal 2") and sigkill in the logs? Why does Apache do this, and what processes are these? Only Windows has these log entries among the buildbots, also in the succeeded test runs.

The script works on timeouts the same way it did before the change. The first http test starts Apache, then at the end of the whole session the script sends a sigterm to Apache (that is why we run these tests last). It does not touch Apache on timeouts as it did't before the change, but my investigation of the logs raised the question in me, maybe at http timeout apache should be restarted, to deal with timeouts caused by strange Apache behaviour.

[Brian Weinstein]

I am still worried about these changes though, because running httpd is now very flaky, and the most recent changes to running http tests have come from this bug. This is something that still needs investigation, because having 300 tests time out after 1 does is not acceptable. I'm trying to figure out where these timeouts started, but the bots don't keep that much history.

[Andras Becsi]

(In reply to comment #86)
> I am still worried about these changes though, because running httpd is now
> very flaky, and the most recent changes to running http tests have come from
> this bug. This is something that still needs investigation, because having 300
> tests time out after 1 does is not acceptable. I'm trying to figure out where
> these timeouts started, but the bots don't keep that much history.

I totally agree, that it is not acceptable, to have 300 timeouts because of one test timeout and it needs investigation, but I think it is more likely to be a sporadic sideeffect in http/tests/security/aboutBlank/xss-DENIED-navigate-opener-document-write.html or in the testing somewhere, because the change in question is in since 11 days and I've only seen these timeouts on the Windows bot since 2 or 3 days. Please let me know what your further investigation revealed.

[Andras Becsi]

(In reply to comment #87)
> (In reply to comment #86)
I filed a bug to track this issue separatelly, because this bug became rather huge: https://bugs.webkit.org/show_bug.cgi?id=34399.

[Eric Seidel (no email)]

Comment on attachment 47012 [details]
Updated patch

Clearing ap's r+ on this obsolete patch.

[Csaba Osztrogonác]

Yesterday night in https://bugs.webkit.org/show_bug.cgi?id=34399 we demonstrated that the refactoring patch from this bug isn't depends on regulary time outs on Windows bots. So I can't find any reason why not to review httpd locking mechanism patch.

[Tor Arne Vestbø]

Comment on attachment 47768 [details]
Rebased patch after r54084

Generally I'd like to see a more fleshed out changelog about what's going on here.

A few style nitpicks:


> +sub filterNumber

I'd rename this extractLockNumber or something along those lines, since you can't use it to filter any number from any string eg.

> +sub getLockFiles
> +{
> +    opendir(TMPDIR, $tmpDir) or die "Could not open " . $tmpDir . ".";
> +    my @lockFiles = grep {m/^$httpdLockPrefix\d+$/} readdir TMPDIR;
> +    @lockFiles = sort { filterNumber($a) <=> filterNumber($b) } @lockFiles;
> +    closedir TMPDIR;
> +    return @lockFiles;
> +}

use readdir() and closedir() to match opendir(), ie don't leave out the parenthesis

> +sub getRunningLockNumber
> +{
> +    my @lockFiles = getLockFiles();
> +    return 0 unless @lockFiles;
> +    return filterNumber($lockFiles[0]);
> +}



> +sub waitForHTTPDLock
> +{
> +    $waitBeginTime = time;
> +    if (scheduleHttpTesting() > 1) {

Refactor scheduleHttpTesting() not return anything, and then do this check in waitForHTTOPD lock, with a comment

> +        my $currentLockFile = File::Spec->catfile($tmpDir, "$httpdLockPrefix".getRunningLockNumber());

space between concat operator .

> +        my $currentLockPid = <LOCKFILE> if (-f $currentLockFile && open(LOCKFILE, "<$currentLockFile"));
> +        # Wait until we are allowed to run the http tests
> +        while($currentLockPid && $currentLockPid != $$) {

space after while (

> +            $currentLockFile = File::Spec->catfile($tmpDir, "$httpdLockPrefix".getRunningLockNumber());

space between concat operator .

> +sub scheduleHttpTesting
> +{
> +    while (!(open(EXLOCK, ">$exclusiveLockFile") && flock(EXLOCK, LOCK_EX|LOCK_NB))) {}

full name for EXLOCK

A comment here would be nice

> +    $myLockFile = File::Spec->catfile($tmpDir, "$httpdLockPrefix".getNextAvailableLockNumber());

space between concat operator .

> +    open(LOCKFILE, ">$myLockFile");
> +    print LOCKFILE "$$";
> +    print EXLOCK "$$";
> +    close(LOCKFILE);
> +    close(EXLOCK);

I'd like to see LOCKFILE and EXLOCK renamed to better describe how they differ. Comments about this in the changelog would also be nice (ie how the two lock files interact)




> +my $shouldWaitForHTTPD = $ENV{"WEBKIT_WAIT_FOR_HTTPD"};

Please comment in src and changelog why this feature is controlled though an ENV variable

[Tor Arne Vestbø]

Comment on attachment 47768 [details]
Rebased patch after r54084

> +sub getRunningLockNumber
> +{
> +    my @lockFiles = getLockFiles();
> +    return 0 unless @lockFiles;
> +    return filterNumber($lockFiles[0]);
> +}

Also, I'd rename this getLockNumberForCurrentRunning (a bit verbose perhaps), or something along those lines. getRunningLockNumber to me means "running" as in give me the next available/running lock number.

[Andras Becsi]

Created attachment 48137 [details]
Updated naming scheme and changelog as discussed

[Tor Arne Vestbø]

Comment on attachment 48137 [details]
Updated naming scheme and changelog as discussed

r=me with these two fixes before landing:

> -my $verbose = 0;
> +my $verbose = 0; 

Whitespace-change, please fix before landing

> +    'wait-for-httpd' => \$shouldWaitForHTTPD,

Should be wait-for-http! to signal it's a boolean flag

[Andras Becsi]

Comment on attachment 48137 [details]
Updated naming scheme and changelog as discussed

Committed in revision 54342. Clearing flags.

[Eric Seidel (no email)]

Adam and I are very interested in this functionality for the EWS bots.  One of the nice things that new-run-webkit-tests does (when running in --chromium mode) is to store the layout test results in the build directory instead of /tmp.  That would also make running more than one run-webkit-tests per machine easier as currently if multiple users run run-webkit-tests one of them is bound to fail due to not being able to access /tmp/layout-test-results.

[Csaba Osztrogonác]

(In reply to comment #96)
You can pass a directory for layout test result with 
(old-)run-webkit-tests with -o option, it works correctly,
so it isn't a problem. 

There are other problems with running more than one run-webkit-tests:

- hard coded /tmp/... in layout tests - very ugly :((
- not process safe local storage, icon database 
(see: https://bugs.webkit.org/show_bug.cgi?id=36899)
- /tmp/LayoutTests symlink to WEBKIT_SOURCE/LayoutTests
- other hard coded temporary file pathes
(see: https://bugs.webkit.org/show_bug.cgi?id=36899)

[Andras Becsi]

We tried multiple aproaches with Ossy to fix the mentioned issues. The non-process safe databases can be fixed with bug 36899, and using the provided per DRT tempdir the ~20 hardcoded /tmp/LayoutTests could also be fixed, but this would need changes to the tests themselves.
The problem is I do not see whether this change would be feasible, because we would need to use js to acquire the path to LayoutTests dir dinamically, and the python script does the whole job without this change, so it might be better to make the python script able to test all platforms rather than hacking the old perl script.

[Eric Seidel (no email)]

I'm confused on how the python script works around the /tmp/ issue?  Maybe Chromium's test_shell redirects loads to /tmp/?

[Dirk Pranke]

(In reply to comment #99)
> I'm confused on how the python script works around the /tmp/ issue?  Maybe
> Chromium's test_shell redirects loads to /tmp/?

I'm not aware of any hard-coded references to /tmp or any code in test_shell to deal with them. That doesn't mean that they're not there, but maybe we don't run the tests and/or have platform-specific versions of them? I will double check some of the tests listed in the first comment and report back.

[Andras Becsi]

Runnig multiple instances on close revisions of the LayoutTests dir should be possible with --wait-for-httpd (multiple Qt bots run on the same machine since Q2 last year), furthermore there is active work going on to make new-run-webkit-tests default (BUG 34984), which should resolve any potentially remaining issues.

Closing this bug as fixed.

[Alexey Proskuryakov]

Is this feature still used by any supported ports? I'd like to remove it if not.

[Csaba Osztrogonác]

(In reply to comment #102)
> Is this feature still used by any supported ports? I'd like to remove it if
> not.

There were many issues fixed by this bug, many of them were needed
for parallel test running used by new-run-webkit-tests too.

Do you mean only removing http file locking mechanism?
There is already a bug report for it: bug136722

[Alexey Proskuryakov]

Yes, file locking was what caught my eye. Thank you for the pointer to an existing bug!

Was there anything else that was only needed for running multiple run-webkit-tests instances, but not for running tests in parallel under a single run-webkit-tests? We should remove those things too, if there are any.

[Csaba Osztrogonác]

(In reply to comment #104)
> Yes, file locking was what caught my eye. Thank you for the pointer to an
> existing bug!
> 
> Was there anything else that was only needed for running multiple
> run-webkit-tests instances, but not for running tests in parallel under a
> single run-webkit-tests? We should remove those things too, if there are any.

I can't remember other bug to support running more run-webkit-tests.
Most of these bugs were related to concurrent access to temporary files,
local storage, databases, etc. by different DRT/WTR. Many of them were port
specific. It is absolutely irrelevant if one or more run-webkit-tests run
these DRT/WTR instances.

[Alexey Proskuryakov]

Yes, those things are all good, and still helpful for running tests in parallel within one RWT instance.