Improve reliability of bulk creation and deletion of access keys (#995)

* wip

* rename file and remove bogus test

* create file library and add unit tests

oh lol

formatting

* switch to node-tmp

* makes constants screaming case

* missed a 'text'

* this is what i get for trying to use the web editor

* roll back title case

src/shadowbox/infrastructure/file.spec.ts

@@ -0,0 +1,63 @@
+import * as fs from 'fs';
+import * as tmp from 'tmp';
+import * as file from './file';
+
+describe('file', () => {
+  tmp.setGracefulCleanup();
+
+  describe('readFileIfExists', () => {
+    let tmpFile: tmp.FileResult;
+
+    beforeEach(() => tmpFile = tmp.fileSync());
+
+    it('reads the file if it exists', () => {
+      const contents = 'test';
+
+      fs.writeFileSync(tmpFile.name, contents);
+
+      expect(file.readFileIfExists(tmpFile.name)).toBe(contents);
+    });
+
+    it('reads the file if it exists and is empty', () => {
+      fs.writeFileSync(tmpFile.name, '');
+
+      expect(file.readFileIfExists(tmpFile.name)).toBe('');
+    });
+
+    it('returns null if file doesn\'t exist',
+       () => expect(file.readFileIfExists(tmp.tmpNameSync())).toBe(null));
+  });
+
+  describe('atomicWriteFileSync', () => {
+    let tmpFile: tmp.FileResult;
+
+    beforeEach(() => tmpFile = tmp.fileSync());
+
+    it('writes to the file', () => {
+      const contents = 'test';
+
+      file.atomicWriteFileSync(tmpFile.name, contents);
+
+      expect(fs.readFileSync(tmpFile.name, {encoding: 'utf8'})).toEqual(contents);
+    });
+
+    it('supports multiple simultaneous writes to the same file', async () => {
+      const writeCount = 100;
+
+      const writer = (_, id) => new Promise<void>((resolve, reject) => {
+        try {
+          file.atomicWriteFileSync(
+              tmpFile.name, `${fs.readFileSync(tmpFile.name, {encoding: 'utf-8'})}${id}\n`);
+          resolve();
+        } catch (e) {
+          reject(e);
+        }
+      });
+
+      await Promise.all(Array.from({length: writeCount}, writer));
+
+      expect(fs.readFileSync(tmpFile.name, {encoding: 'utf8'}).trimEnd().split('\n').length)
+          .toBe(writeCount);
+    });
+  });
+});

src/shadowbox/infrastructure/file.ts

@@ -18,7 +18,7 @@ import * as fs from 'fs';
 // Throws any other error except file not found.
 export function readFileIfExists(filename: string): string {
   try {
-    return fs.readFileSync(filename, {encoding: 'utf8'}) || null;
+    return fs.readFileSync(filename, {encoding: 'utf8'}) ?? null;
   } catch (err) {
     // err.code will be 'ENOENT' if the file is not found, this is expected.
     if (err.code === 'ENOENT') {
@@ -28,3 +28,13 @@ export function readFileIfExists(filename: string): string {
     }
   }
 }
+
+// Write to temporary file, then move that temporary file to the
+// persistent location, to avoid accidentally breaking the metrics file.
+// Use *Sync calls for atomic operations, to guard against corrupting
+// these files.
+export function atomicWriteFileSync(filename: string, filebody: string) {
+  const tempFilename = `${filename}.${Date.now()}`;
+  fs.writeFileSync(tempFilename, filebody, {encoding: 'utf8'});
+  fs.renameSync(tempFilename, filename);
+}

src/shadowbox/infrastructure/json_config.ts

@@ -12,9 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import * as fs from 'fs';
-
-import * as file_read from './file_read';
+import * as file from './file';
 import * as logging from './logging';
 
 export interface JsonConfig<T> {
@@ -25,7 +23,7 @@ export interface JsonConfig<T> {
 }
 
 export function loadFileConfig<T>(filename: string): JsonConfig<T> {
-  const text = file_read.readFileIfExists(filename);
+  const text = file.readFileIfExists(filename);
   let dataJson = {} as T;
   if (text) {
     dataJson = JSON.parse(text) as T;
@@ -33,6 +31,7 @@ export function loadFileConfig<T>(filename: string): JsonConfig<T> {
   return new FileConfig<T>(filename, dataJson);
 }
 
+
 // FileConfig is a JsonConfig backed by a filesystem file.
 export class FileConfig<T> implements JsonConfig<T> {
   constructor(private filename: string, private dataJson: T) {}
@@ -42,14 +41,8 @@ export class FileConfig<T> implements JsonConfig<T> {
   }
 
   write() {
-    // Write to temporary file, then move that temporary file to the
-    // persistent location, to avoid accidentally breaking the metrics file.
-    // Use *Sync calls for atomic operations, to guard against corrupting
-    // these files.
-    const tempFilename = `${this.filename}.${Date.now()}`;
     try {
-      fs.writeFileSync(tempFilename, JSON.stringify(this.dataJson), {encoding: 'utf8'});
-      fs.renameSync(tempFilename, this.filename);
+      file.atomicWriteFileSync(this.filename, JSON.stringify(this.dataJson));
     } catch (error) {
       // TODO: Stop swallowing the exception and handle it in the callers.
       logging.error(`Error writing config ${this.filename} ${error}`);

src/shadowbox/package.json

@@ -28,6 +28,8 @@
     "@types/randomstring": "^1.1.6",
     "@types/restify": "^8.4.2",
     "@types/restify-cors-middleware": "^1.0.1",
+    "@types/tmp": "^0.2.1",
+    "tmp": "^0.2.1",
     "ts-loader": "^7.0.4",
     "webpack": "^4.43.0",
     "webpack-cli": "^3.3.11"

src/shadowbox/server/outline_shadowsocks_server.ts

@@ -13,11 +13,11 @@
 // limitations under the License.
 
 import * as child_process from 'child_process';
-import * as fs from 'fs';
 import * as jsyaml from 'js-yaml';
 import * as mkdirp from 'mkdirp';
 import * as path from 'path';
 
+import * as file from '../infrastructure/file';
 import * as logging from '../infrastructure/logging';
 import {ShadowsocksAccessKey, ShadowsocksServer} from '../model/shadowsocks_server';
 
@@ -68,16 +68,18 @@ export class OutlineShadowsocksServer implements ShadowsocksServer {
               key.id} is not supported: use an AEAD cipher instead.`);
           continue;
         }
+
         keysJson.keys.push(key);
       }
-      const ymlTxt = jsyaml.safeDump(keysJson, {'sortKeys': true});
+
       mkdirp.sync(path.dirname(this.configFilename));
-      fs.writeFile(this.configFilename, ymlTxt, 'utf-8', (err) => {
-        if (err) {
-          reject(err);
-        }
+
+      try {
+        file.atomicWriteFileSync(this.configFilename, jsyaml.safeDump(keysJson, {sortKeys: true}));
         resolve();
-      });
+      } catch (error) {
+        reject(error);
+      }
     });
   }
 

yarn.lock

@@ -1038,6 +1038,11 @@
   resolved "https://registry.yarnpkg.com/@types/tapable/-/tapable-1.0.8.tgz#b94a4391c85666c7b73299fd3ad79d4faa435310"
   integrity sha512-ipixuVrh2OdNmauvtT51o3d8z12p6LtFW9in7U79der/kwejjdNchQC5UMn5u/KxNoM7VHHOs/l8KS8uHxhODQ==
 
+"@types/tmp@^0.2.1":
+  version "0.2.1"
+  resolved "https://registry.yarnpkg.com/@types/tmp/-/tmp-0.2.1.tgz#83ecf4ec22a8c218c71db25f316619fe5b986011"
+  integrity sha512-7cTXwKP/HLOPVgjg+YhBdQ7bMiobGMuoBmrGmqwIWJv8elC6t1DfVc/mn4fD9UE1IjhwmhaQ5pGVXkmXbH0rhg==
+
 "@types/tough-cookie@*":
   version "4.0.1"
   resolved "https://registry.yarnpkg.com/@types/tough-cookie/-/tough-cookie-4.0.1.tgz#8f80dd965ad81f3e1bc26d6f5c727e132721ff40"
@@ -9969,7 +9974,7 @@ tinycolor2@^1.1.2:
   resolved "https://registry.yarnpkg.com/tinycolor2/-/tinycolor2-1.4.2.tgz#3f6a4d1071ad07676d7fa472e1fac40a719d8803"
   integrity sha512-vJhccZPs965sV/L2sU4oRQVAos0pQXwsvTLkWYdqJ+a8Q5kPFzJTuOFwy7UniPli44NKQGAglksjvOcpo95aZA==
 
-tmp@0.2.1:
+tmp@0.2.1, tmp@^0.2.1:
   version "0.2.1"
   resolved "https://registry.yarnpkg.com/tmp/-/tmp-0.2.1.tgz#8457fc3037dcf4719c251367a1af6500ee1ccf14"
   integrity sha512-76SUhtfqR2Ijn+xllcI5P1oyannHNHByD80W1q447gU3mp9G9PSpGdWmjUOHRDPiHYacIk66W7ubDTuPF3BEtQ==