From 73bac54ed2838fb7bfd1986bdbc4c8d5bb09e335 Mon Sep 17 00:00:00 2001 From: Isaiah Odhner Date: Fri, 30 Mar 2018 17:38:42 -0400 Subject: [PATCH 1/4] WIP: remove most callbacks from try blocks Fix doubled callbacks, called once to handle results and then a second time to handle an error thrown in the callback Does not compile! Yet to do FS.ts Need to revisit: * AsyncMirror.Create * IndexedDBROTransaction.get * IndexedDBRWTransaction.put, .del * IndexedDBStore.clear * IndexedDBFileSystem.create * OverlayFS.Create * ZipFS.Create * ZipFS._computeIndex --- src/backend/Emscripten.ts | 24 +++++++--- src/backend/HTTPRequest.ts | 16 +++++-- src/backend/IndexedDB.ts | 6 +-- src/backend/ZipFS.ts | 10 ++++- src/core/file_system.ts | 68 +++++++++++++++++------------ src/generic/key_value_filesystem.ts | 7 ++- src/generic/preload_file.ts | 32 ++++++++------ src/generic/xhr.ts | 4 +- 8 files changed, 108 insertions(+), 59 deletions(-) diff --git a/src/backend/Emscripten.ts b/src/backend/Emscripten.ts index faa751c6..380c9664 100644 --- a/src/backend/Emscripten.ts +++ b/src/backend/Emscripten.ts @@ -60,10 +60,14 @@ export class EmscriptenFile extends BaseFile implements File { } } public stat(cb: BFSCallback): void { + let res; + let err: ApiError | null = null; try { - cb(null, this.statSync()); + res = this.statSync(); } catch (e) { - cb(e); + err = e; + } finally { + cb(err, res); } } public statSync(): Stats { @@ -91,10 +95,14 @@ export class EmscriptenFile extends BaseFile implements File { } } public write(buffer: NodeBuffer, offset: number, length: number, position: number, cb: BFSThreeArgCallback): void { + let res; + let err: ApiError | null = null; try { - cb(null, this.writeSync(buffer, offset, length, position), buffer); + res = this.writeSync(buffer, offset, length, position); } catch (e) { - cb(e); + err = e; + } finally { + cb(err, res, buffer); } } public writeSync(buffer: NodeBuffer, offset: number, length: number, position: number | null): number { @@ -108,10 +116,14 @@ export class EmscriptenFile extends BaseFile implements File { } } public read(buffer: NodeBuffer, offset: number, length: number, position: number, cb: BFSThreeArgCallback): void { + let res; + let err: ApiError | null = null; try { - cb(null, this.readSync(buffer, offset, length, position), buffer); + res = this.readSync(buffer, offset, length, position); } catch (e) { - cb(e); + err = e; + } finally { + cb(err, res, buffer); } } public readSync(buffer: NodeBuffer, offset: number, length: number, position: number | null): number { diff --git a/src/backend/HTTPRequest.ts b/src/backend/HTTPRequest.ts index 1f5688a9..83c922ed 100644 --- a/src/backend/HTTPRequest.ts +++ b/src/backend/HTTPRequest.ts @@ -16,10 +16,14 @@ import {FileIndex, isFileInode, isDirInode} from '../generic/file_index'; * @hidden */ function tryToString(buff: Buffer, encoding: string, cb: BFSCallback) { + let string: string | null = null; + let err: ApiError | null = null; try { - cb(null, buff.toString(encoding)); + string = buff.toString(encoding); } catch (e) { - cb(e); + err = e; + } finally { + cb(err, string); } } @@ -339,10 +343,14 @@ export default class HTTPRequest extends BaseFileSystem implements FileSystem { } public readdir(path: string, cb: BFSCallback): void { + let res; + let err: ApiError | null = null; try { - cb(null, this.readdirSync(path)); + res = this.readdirSync(path); } catch (e) { - cb(e); + err = e; + } finally { + cb(err, res); } } diff --git a/src/backend/IndexedDB.ts b/src/backend/IndexedDB.ts index d8765f22..74a7344a 100644 --- a/src/backend/IndexedDB.ts +++ b/src/backend/IndexedDB.ts @@ -115,13 +115,13 @@ export class IndexedDBRWTransaction extends IndexedDBROTransaction implements As } public abort(cb: BFSOneArgCallback): void { - let _e: ApiError | null = null; + let err: ApiError | null = null; try { this.tx.abort(); } catch (e) { - _e = convertError(e); + err = convertError(e); } finally { - cb(_e); + cb(err); } } } diff --git a/src/backend/ZipFS.ts b/src/backend/ZipFS.ts index c68231ab..06f5efb9 100644 --- a/src/backend/ZipFS.ts +++ b/src/backend/ZipFS.ts @@ -626,11 +626,17 @@ export default class ZipFS extends SynchronousFileSystem implements FileSystem { } private static _computeIndexResponsiveTrampoline(data: Buffer, index: FileIndex, cdPtr: number, cdEnd: number, cb: BFSCallback, cdEntries: CentralDirectory[], eocd: EndOfCentralDirectory) { + let err: ApiError | null = null; + let zipTOC: ZipTOC | null = null; try { - ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, cb, cdEntries, eocd); + ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, (_err: ApiError | null, _zipTOC: ZipTOC | null)=> { + err = _err; + zipTOC = _zipTOC; + }, cdEntries, eocd); } catch (e) { - cb(e); + return cb(e); } + cb(err, zipTOC); } private static _computeIndexResponsive(data: Buffer, index: FileIndex, cdPtr: number, cdEnd: number, cb: BFSCallback, cdEntries: CentralDirectory[], eocd: EndOfCentralDirectory) { diff --git a/src/core/file_system.ts b/src/core/file_system.ts index 1de93a21..8101161f 100644 --- a/src/core/file_system.ts +++ b/src/core/file_system.ts @@ -660,10 +660,14 @@ export class BaseFileSystem { } else if (encoding === null) { return cb(err, buf); } + // TODO: Share tryToString helper with HTTPRequest.ts + let string: string; try { - cb(null, buf.toString(encoding)); + string = buf.toString(encoding); } catch (e) { - cb(e); + err = e; + } finally { + cb(err, string); } }); }); @@ -803,113 +807,121 @@ export class SynchronousFileSystem extends BaseFileSystem { public rename(oldPath: string, newPath: string, cb: BFSOneArgCallback): void { try { this.renameSync(oldPath, newPath); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public stat(p: string, isLstat: boolean | null, cb: BFSCallback): void { + let res: Stats | null; try { - cb(null, this.statSync(p, isLstat)); + res = this.statSync(p, isLstat); } catch (e) { - cb(e); + return cb(e); } + cb(null, res); } public open(p: string, flags: FileFlag, mode: number, cb: BFSCallback): void { + let res: File | null; try { - cb(null, this.openSync(p, flags, mode)); + res = this.openSync(p, flags, mode); } catch (e) { - cb(e); + return cb(e); } + cb(null, res); } public unlink(p: string, cb: BFSOneArgCallback): void { try { this.unlinkSync(p); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public rmdir(p: string, cb: BFSOneArgCallback): void { try { this.rmdirSync(p); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public mkdir(p: string, mode: number, cb: BFSOneArgCallback): void { try { this.mkdirSync(p, mode); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public readdir(p: string, cb: BFSCallback): void { + let res: string[] | null; try { - cb(null, this.readdirSync(p)); + res = this.readdirSync(p); } catch (e) { - cb(e); + return cb(e); } + cb(null, res); } public chmod(p: string, isLchmod: boolean, mode: number, cb: BFSOneArgCallback): void { try { this.chmodSync(p, isLchmod, mode); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public chown(p: string, isLchown: boolean, uid: number, gid: number, cb: BFSOneArgCallback): void { try { this.chownSync(p, isLchown, uid, gid); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public utimes(p: string, atime: Date, mtime: Date, cb: BFSOneArgCallback): void { try { this.utimesSync(p, atime, mtime); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public link(srcpath: string, dstpath: string, cb: BFSOneArgCallback): void { try { this.linkSync(srcpath, dstpath); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public symlink(srcpath: string, dstpath: string, type: string, cb: BFSOneArgCallback): void { try { this.symlinkSync(srcpath, dstpath, type); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } public readlink(p: string, cb: BFSCallback): void { + let res: string | null; try { - cb(null, this.readlinkSync(p)); + res = this.readlinkSync(p); } catch (e) { - cb(e); + return cb(e); } + cb(null, res); } } diff --git a/src/generic/key_value_filesystem.ts b/src/generic/key_value_filesystem.ts index 2652807c..994f6cef 100644 --- a/src/generic/key_value_filesystem.ts +++ b/src/generic/key_value_filesystem.ts @@ -1087,14 +1087,17 @@ export class AsyncKeyValueFileSystem extends BaseFileSystem { } else { tx.get(inode.id, (e: ApiError, data?: Buffer): void => { if (noError(e, cb)) { + let err: ApiError | null; + let res; try { - cb(null, JSON.parse(data!.toString())); + res = JSON.parse(data!.toString()); } catch (e) { // Occurs when data is undefined, or corresponds to something other // than a directory listing. The latter should never occur unless // the file system is corrupted. - cb(ApiError.ENOENT(p)); + err = ApiError.ENOENT(p); } + cb(err, res); } }); } diff --git a/src/generic/preload_file.ts b/src/generic/preload_file.ts index 5af03a22..c25a650e 100644 --- a/src/generic/preload_file.ts +++ b/src/generic/preload_file.ts @@ -119,10 +119,10 @@ export default class PreloadFile extends BaseFile { public sync(cb: BFSOneArgCallback): void { try { this.syncSync(); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } /** @@ -140,10 +140,10 @@ export default class PreloadFile extends BaseFile { public close(cb: BFSOneArgCallback): void { try { this.closeSync(); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } /** @@ -158,11 +158,13 @@ export default class PreloadFile extends BaseFile { * @param [Function(BrowserFS.ApiError, BrowserFS.node.fs.Stats)] cb */ public stat(cb: BFSCallback): void { + let res: Stats; try { - cb(null, Stats.clone(this._stat)); + res = Stats.clone(this._stat); } catch (e) { - cb(e); + return cb(e); } + cb(null, res); } /** @@ -183,10 +185,10 @@ export default class PreloadFile extends BaseFile { if (this._flag.isSynchronous() && !fs.getRootFS()!.supportsSynch()) { this.sync(cb); } - cb(); } catch (e) { return cb(e); } + cb(); } /** @@ -233,11 +235,13 @@ export default class PreloadFile extends BaseFile { * cb The number specifies the number of bytes written into the file. */ public write(buffer: Buffer, offset: number, length: number, position: number, cb: BFSThreeArgCallback): void { + let res: number; try { - cb(null, this.writeSync(buffer, offset, length, position), buffer); + res = this.writeSync(buffer, offset, length, position); } catch (e) { - cb(e); + return cb(e); } + cb(null, res, buffer); } /** @@ -295,11 +299,13 @@ export default class PreloadFile extends BaseFile { * number is the number of bytes read */ public read(buffer: Buffer, offset: number, length: number, position: number, cb: BFSThreeArgCallback): void { + let res: number; try { - cb(null, this.readSync(buffer, offset, length, position), buffer); + res = this.readSync(buffer, offset, length, position); } catch (e) { - cb(e); + return cb(e); } + cb(null, res, buffer); } /** @@ -339,10 +345,10 @@ export default class PreloadFile extends BaseFile { public chmod(mode: number, cb: BFSOneArgCallback): void { try { this.chmodSync(mode); - cb(); } catch (e) { - cb(e); + return cb(e); } + cb(); } /** diff --git a/src/generic/xhr.ts b/src/generic/xhr.ts index 6f3461f0..0d9d58ff 100644 --- a/src/generic/xhr.ts +++ b/src/generic/xhr.ts @@ -162,12 +162,14 @@ function getFileSize(async: boolean, p: string, cb: BFSCallback): void { req.onreadystatechange = function(e) { if (req.readyState === 4) { if (req.status === 200) { + let contentLength; try { - return cb(null, parseInt(req.getResponseHeader('Content-Length') || '-1', 10)); + contentLength = parseInt(req.getResponseHeader('Content-Length') || '-1', 10); } catch (e) { // In the event that the header isn't present or there is an error... return cb(new ApiError(ErrorCode.EIO, "XHR HEAD error: Could not read content-length.")); } + return cb(null, contentLength); } else { return cb(new ApiError(ErrorCode.EIO, `XHR HEAD error: response returned code ${req.status}`)); } From 52cbdbf1119775fd3be114ba66854a11aa6a0d37 Mon Sep 17 00:00:00 2001 From: Isaiah Odhner Date: Fri, 30 Mar 2018 19:06:37 -0400 Subject: [PATCH 2/4] Fix type errors --- src/backend/HTTPRequest.ts | 6 +++--- src/backend/ZipFS.ts | 7 ++++--- src/core/file_system.ts | 6 +++--- src/generic/key_value_filesystem.ts | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/backend/HTTPRequest.ts b/src/backend/HTTPRequest.ts index 83c922ed..532ca819 100644 --- a/src/backend/HTTPRequest.ts +++ b/src/backend/HTTPRequest.ts @@ -16,14 +16,14 @@ import {FileIndex, isFileInode, isDirInode} from '../generic/file_index'; * @hidden */ function tryToString(buff: Buffer, encoding: string, cb: BFSCallback) { - let string: string | null = null; + let res: string | undefined; let err: ApiError | null = null; try { - string = buff.toString(encoding); + res = buff.toString(encoding); } catch (e) { err = e; } finally { - cb(err, string); + cb(err, res); } } diff --git a/src/backend/ZipFS.ts b/src/backend/ZipFS.ts index 06f5efb9..cba6b171 100644 --- a/src/backend/ZipFS.ts +++ b/src/backend/ZipFS.ts @@ -627,12 +627,13 @@ export default class ZipFS extends SynchronousFileSystem implements FileSystem { private static _computeIndexResponsiveTrampoline(data: Buffer, index: FileIndex, cdPtr: number, cdEnd: number, cb: BFSCallback, cdEntries: CentralDirectory[], eocd: EndOfCentralDirectory) { let err: ApiError | null = null; - let zipTOC: ZipTOC | null = null; + let zipTOC: ZipTOC | undefined; try { - ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, (_err: ApiError | null, _zipTOC: ZipTOC | null)=> { + let callbackWrapper: BFSCallback = (_err: ApiError, _zipTOC: ZipTOC)=> { err = _err; zipTOC = _zipTOC; - }, cdEntries, eocd); + }; + ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, callbackWrapper, cdEntries, eocd); } catch (e) { return cb(e); } diff --git a/src/core/file_system.ts b/src/core/file_system.ts index 8101161f..584beb70 100644 --- a/src/core/file_system.ts +++ b/src/core/file_system.ts @@ -661,13 +661,13 @@ export class BaseFileSystem { return cb(err, buf); } // TODO: Share tryToString helper with HTTPRequest.ts - let string: string; + let res: string | undefined; try { - string = buf.toString(encoding); + res = buf.toString(encoding); } catch (e) { err = e; } finally { - cb(err, string); + cb(err, res); } }); }); diff --git a/src/generic/key_value_filesystem.ts b/src/generic/key_value_filesystem.ts index 994f6cef..88f958b9 100644 --- a/src/generic/key_value_filesystem.ts +++ b/src/generic/key_value_filesystem.ts @@ -1087,7 +1087,7 @@ export class AsyncKeyValueFileSystem extends BaseFileSystem { } else { tx.get(inode.id, (e: ApiError, data?: Buffer): void => { if (noError(e, cb)) { - let err: ApiError | null; + let err: ApiError | null = null; let res; try { res = JSON.parse(data!.toString()); From 2da9e4eba77664f3fce75ea1c21cc31aaa08c846 Mon Sep 17 00:00:00 2001 From: Isaiah Odhner Date: Fri, 30 Mar 2018 19:14:36 -0400 Subject: [PATCH 3/4] Fix tslint errors --- src/backend/ZipFS.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/ZipFS.ts b/src/backend/ZipFS.ts index cba6b171..cb56d003 100644 --- a/src/backend/ZipFS.ts +++ b/src/backend/ZipFS.ts @@ -629,7 +629,7 @@ export default class ZipFS extends SynchronousFileSystem implements FileSystem { let err: ApiError | null = null; let zipTOC: ZipTOC | undefined; try { - let callbackWrapper: BFSCallback = (_err: ApiError, _zipTOC: ZipTOC)=> { + const callbackWrapper: BFSCallback = (_err: ApiError, _zipTOC: ZipTOC) => { err = _err; zipTOC = _zipTOC; }; From 8f0e576312dc60f40fd011bbce10fb0da942951b Mon Sep 17 00:00:00 2001 From: Isaiah Odhner Date: Wed, 2 May 2018 18:15:44 -0400 Subject: [PATCH 4/4] Revert changes to ZipFS callbacks --- src/backend/ZipFS.ts | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/backend/ZipFS.ts b/src/backend/ZipFS.ts index cb56d003..c68231ab 100644 --- a/src/backend/ZipFS.ts +++ b/src/backend/ZipFS.ts @@ -626,18 +626,11 @@ export default class ZipFS extends SynchronousFileSystem implements FileSystem { } private static _computeIndexResponsiveTrampoline(data: Buffer, index: FileIndex, cdPtr: number, cdEnd: number, cb: BFSCallback, cdEntries: CentralDirectory[], eocd: EndOfCentralDirectory) { - let err: ApiError | null = null; - let zipTOC: ZipTOC | undefined; try { - const callbackWrapper: BFSCallback = (_err: ApiError, _zipTOC: ZipTOC) => { - err = _err; - zipTOC = _zipTOC; - }; - ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, callbackWrapper, cdEntries, eocd); + ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, cb, cdEntries, eocd); } catch (e) { - return cb(e); + cb(e); } - cb(err, zipTOC); } private static _computeIndexResponsive(data: Buffer, index: FileIndex, cdPtr: number, cdEnd: number, cb: BFSCallback, cdEntries: CentralDirectory[], eocd: EndOfCentralDirectory) {