[webkit-reviews] review denied: [Bug 33153] multi-patch: Running multiple instances of run-webkit-tests is not possible on the same machine : [Attachment 47768] Rebased patch after r54084

[bugzilla-daemon at webkit.org]

Tor Arne Vestbø <vestbo at webkit.org> has denied Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 33153: multi-patch: Running multiple instances of run-webkit-tests is not
possible on the same machine
https://bugs.webkit.org/show_bug.cgi?id=33153

Attachment 47768: Rebased patch after r54084
https://bugs.webkit.org/attachment.cgi?id=47768&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
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