Apache can return a corrupt manifest file while ManifestGenerator::store is running
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Sep 2018 05:12:13 +0000 (05:12 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Sep 2018 05:12:13 +0000 (05:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189822

Reviewed by Ryosuke Niwa.

Updating a file on performance dashboard should be transactional between php and apache.
Otherwise, partial content may be served.

* public/api/measurement-set.php: Adapted invocation of 'generate_json_date_with_elapsed_time_if_needed'.
* public/api/runs.php: Adapted invocation of 'generate_json_date_with_elapsed_time_if_needed'.
* public/include/db.php: Make creating file transactionaly by taking advantage of
'move/rename' operation is atomic in OS.
Added logic to avoid overwriting file if all content except specified filed are identical.
* public/include/json-header.php: Added a helper function that added 'status' but does not
convert content to json.
* public/include/manifest-generator.php: Adapted invocation of 'generate_json_date_with_elapsed_time_if_needed'.
* public/shared/statistics.js: Removed unnecessary logging.
(Statistics.new.sampleMeanAndVarianceFromMultipleSamples):
* server-tests/api-manifest-tests.js: Updated one unit test.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236454 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/api/measurement-set.php
Websites/perf.webkit.org/public/api/runs.php
Websites/perf.webkit.org/public/include/db.php
Websites/perf.webkit.org/public/include/json-header.php
Websites/perf.webkit.org/public/include/manifest-generator.php
Websites/perf.webkit.org/public/shared/statistics.js
Websites/perf.webkit.org/server-tests/api-manifest-tests.js

index e0eac5f..b76ddc7 100644 (file)
@@ -1,3 +1,25 @@
+2018-09-21  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Apache can return a corrupt manifest file while ManifestGenerator::store is running
+        https://bugs.webkit.org/show_bug.cgi?id=189822
+
+        Reviewed by Ryosuke Niwa.
+
+        Updating a file on performance dashboard should be transactional between php and apache.
+        Otherwise, partial content may be served.
+
+        * public/api/measurement-set.php: Adapted invocation of 'generate_json_date_with_elapsed_time_if_needed'.
+        * public/api/runs.php: Adapted invocation of 'generate_json_date_with_elapsed_time_if_needed'.
+        * public/include/db.php: Make creating file transactionaly by taking advantage of
+        'move/rename' operation is atomic in OS.
+        Added logic to avoid overwriting file if all content except specified filed are identical.
+        * public/include/json-header.php: Added a helper function that added 'status' but does not
+        convert content to json.
+        * public/include/manifest-generator.php: Adapted invocation of 'generate_json_date_with_elapsed_time_if_needed'.
+        * public/shared/statistics.js: Removed unnecessary logging.
+        (Statistics.new.sampleMeanAndVarianceFromMultipleSamples):
+        * server-tests/api-manifest-tests.js: Updated one unit test.
+
 2018-08-22  Dewei Zhu  <dewei_zhu@apple.com>
 
         Show t-test results based on individual measurements to analysis task page.
index 579bb52..dd954f5 100644 (file)
@@ -37,18 +37,19 @@ function main() {
     }
 
     $cluster_count = 0;
+    $elapsed_time = NULL;
     while (!$fetcher->at_end()) {
         $content = $fetcher->fetch_next_cluster();
         $cluster_count++;
         if ($fetcher->at_end()) {
             $cache_filename = "measurement-set-$platform_id-$metric_id.json";
             $content['clusterCount'] = $cluster_count;
-            $content['elapsedTime'] = (microtime(true) - $program_start_time) * 1000;
+            $elapsed_time = (microtime(true) - $program_start_time) * 1000;
         } else
             $cache_filename = "measurement-set-$platform_id-$metric_id-{$content['endTime']}.json";
 
-        $json = success_json($content);
-        generate_data_file($cache_filename, $json);
+        set_successful($content);
+        $json = generate_json_data_with_elapsed_time_if_needed($cache_filename, $content, $elapsed_time);
     }
 
     echo $json;
index b658e2f..ba044b5 100644 (file)
@@ -39,9 +39,13 @@ function main($path) {
     foreach ($config_rows as $config)
         $generator->fetch_runs($config['config_type'], $config['config_id'], $config['config_runs_last_modified']);
 
-    $content = success_json($generator->results());
+    $results = set_successful($generator->results());
     if (!$test_group_id)
-        generate_data_file("$platform_id-$metric_id.json", $content);
+        $content = generate_json_data_with_elapsed_time_if_needed("$platform_id-$metric_id.json", $results,  $generator->elapsed_time);
+    else {
+        $results['elapsedTime'] = $generator->elapsed_time;
+        $content = json_encode($results);
+    }
     echo $content;
 }
 
@@ -51,13 +55,15 @@ class RunsGenerator {
         $this->results = array();
         $this->last_modified = 0;
         $this->start_time = microtime(true);
+        $this->elapsed_time = NULL;
     }
 
     function results() {
+        $this->elapsed_time = (microtime(true) - $this->start_time) * 1000;
         return array(
             'configurations' => &$this->results,
-            'lastModified' => $this->last_modified,
-            'elapsedTime' => (microtime(true) - $this->start_time) * 1000);
+            'lastModified' => $this->last_modified
+        );
     }
 
     function fetch_runs($name, $config_id, $last_modified) {
index ed0e1d8..ada95ed 100644 (file)
@@ -44,10 +44,33 @@ function config_path($key, $path) {
     return CONFIG_DIR . '/' . config($key) . '/' . $path;
 }
 
-function generate_data_file($filename, $content) {
-    if (!assert(ctype_alnum(str_replace(array('-', '_', '.'), '', $filename))))
+function same_till_last_occurrence_of_string($main_string, $comparing_string, $compare_to_last_position_string) {
+    if (!$compare_to_last_position_string)
+        return !strcmp($main_string, $comparing_string);
+
+    $position_in_main_string = strrpos($main_string, $compare_to_last_position_string);
+    $position_in_comparing_string = strrpos($comparing_string, $compare_to_last_position_string);
+    if ($position_in_main_string !== $position_in_comparing_string)
         return FALSE;
-    return file_put_contents(config_path('dataDirectory', $filename), $content, LOCK_EX);
+    if ($position_in_main_string === FALSE)
+        return !strcmp($main_string, $comparing_string);
+    return !substr_compare($main_string, $comparing_string, 0, $position_in_main_string);
+}
+
+function generate_json_data_with_elapsed_time_if_needed($filename, $object, $elapsed_time) {
+    if (!assert(ctype_alnum(str_replace(array('-', '_', '.'), '', $filename))))
+        return NULL;
+
+    $target_file_path = config_path('dataDirectory', $filename);
+    $object['elapsedTime'] = $elapsed_time;
+    $content = json_encode($object);
+
+    if (file_exists($target_file_path) && same_till_last_occurrence_of_string(file_get_contents($target_file_path), $content, 'elapsedTime'))
+        return $content;
+
+    $temp_file_path = tempnam(config_path('dataDirectory', ''), $filename . '-temp-');
+    file_put_contents($temp_file_path, $content, LOCK_EX);
+    return rename($temp_file_path, $target_file_path) ?  $content : NULL;
 }
 
 if (config('debug')) {
index 5f8cf14..a27641c 100644 (file)
@@ -14,10 +14,14 @@ function exit_with_error($status, $details = array()) {
     exit(1);
 }
 
-function success_json($details = array()) {
+function set_successful(&$details = array()) {
     $details['status'] = 'OK';
     merge_additional_details($details);
+    return $details;
+}
 
+function success_json($details = array()) {
+    set_successful($details);
     return json_encode($details);
 }
 
index e5436e3..bafd825 100644 (file)
@@ -9,6 +9,7 @@ class ManifestGenerator {
 
     function __construct($db) {
         $this->db = $db;
+        $this->elapsed_time = NULL;
     }
 
     function generate() {
@@ -47,7 +48,7 @@ class ManifestGenerator {
             'testAgeToleranceInHours' => config('testAgeToleranceInHours'),
         );
 
-        $this->manifest['elapsedTime'] = (microtime(true) - $start_time) * 1000;
+        $this->elapsed_time = (microtime(true) - $start_time) * 1000;
 
         return TRUE;
     }
@@ -55,7 +56,7 @@ class ManifestGenerator {
     function manifest() { return $this->manifest; }
 
     function store() {
-        return generate_data_file('manifest.json', json_encode($this->manifest));
+        return generate_json_data_with_elapsed_time_if_needed('manifest.json', $this->manifest, $this->elapsed_time);
     }
 
     private function tests() {
index 871846c..3228ea9 100644 (file)
@@ -72,7 +72,6 @@ var Statistics = new (function () {
         let sum = 0;
         let squareSum = 0;
         let size = 0;
-        console.log(samples);
         for (const sample of samples) {
             sum += sample.sum;
             squareSum += sample.squareSum;
index cb5fc55..275a429 100644 (file)
@@ -14,10 +14,7 @@ describe('/api/manifest', function () {
     it("should generate an empty manifest when database is empty", () => {
         return TestServer.remoteAPI().getJSON('/api/manifest').then((manifest) => {
             assert.deepEqual(Object.keys(manifest).sort(), ['all', 'bugTrackers', 'builders', 'dashboard', 'dashboards',
-                'elapsedTime', 'fileUploadSizeLimit', 'metrics', 'repositories', 'siteTitle', 'status', 'summaryPages', 'testAgeToleranceInHours', 'tests', 'triggerables']);
-
-            assert.equal(typeof(manifest.elapsedTime), 'number');
-            delete manifest.elapsedTime;
+                'fileUploadSizeLimit', 'metrics', 'repositories', 'siteTitle', 'status', 'summaryPages', 'testAgeToleranceInHours', 'tests', 'triggerables']);
 
             assert.deepStrictEqual(manifest, {
                 siteTitle: TestServer.testConfig().siteTitle,