Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fix caught callbacks #223

Closed
wants to merge 4 commits into from
Closed

[WIP] Fix caught callbacks #223

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
73bac54
WIP: remove most callbacks from try blocks
1j01 Mar 30, 2018
52cbdbf
Fix type errors
1j01 Mar 30, 2018
2da9e4e
Fix tslint errors
1j01 Mar 30, 2018
8f0e576
Revert changes to ZipFS callbacks
1j01 May 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions src/backend/Emscripten.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,14 @@ export class EmscriptenFile extends BaseFile implements File {
}
}
public stat(cb: BFSCallback<Stats>): 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 {
Expand Down Expand Up @@ -91,10 +95,14 @@ export class EmscriptenFile extends BaseFile implements File {
}
}
public write(buffer: NodeBuffer, offset: number, length: number, position: number, cb: BFSThreeArgCallback<number, Buffer>): 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 {
Expand All @@ -108,10 +116,14 @@ export class EmscriptenFile extends BaseFile implements File {
}
}
public read(buffer: NodeBuffer, offset: number, length: number, position: number, cb: BFSThreeArgCallback<number, Buffer>): 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 {
Expand Down
16 changes: 12 additions & 4 deletions src/backend/HTTPRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@ import {FileIndex, isFileInode, isDirInode} from '../generic/file_index';
* @hidden
*/
function tryToString(buff: Buffer, encoding: string, cb: BFSCallback<string>) {
let res: string | undefined;
let err: ApiError | null = null;
try {
cb(null, buff.toString(encoding));
res = buff.toString(encoding);
} catch (e) {
cb(e);
err = e;
} finally {
cb(err, res);
}
}

Expand Down Expand Up @@ -339,10 +343,14 @@ export default class HTTPRequest extends BaseFileSystem implements FileSystem {
}

public readdir(path: string, cb: BFSCallback<string[]>): 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);
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/backend/IndexedDB.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
11 changes: 9 additions & 2 deletions src/backend/ZipFS.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,11 +626,18 @@ export default class ZipFS extends SynchronousFileSystem implements FileSystem {
}

private static _computeIndexResponsiveTrampoline(data: Buffer, index: FileIndex<CentralDirectory>, cdPtr: number, cdEnd: number, cb: BFSCallback<ZipTOC>, cdEntries: CentralDirectory[], eocd: EndOfCentralDirectory) {
let err: ApiError | null = null;
let zipTOC: ZipTOC | undefined;
try {
ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, cb, cdEntries, eocd);
const callbackWrapper: BFSCallback<ZipTOC> = (_err: ApiError, _zipTOC: ZipTOC) => {
err = _err;
zipTOC = _zipTOC;
};
ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, callbackWrapper, cdEntries, eocd);
Copy link
Contributor Author

@1j01 1j01 May 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm relying on _computeIndexResponsive calling back synchronously, but it's not actually always synchronous. If it was always async, I think the callback in try would be okay, but would warrant a comment like // MUST be asynchronous to avoid a double callback in _computeIndexResponsiveTrampoline

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see; yeah, it's not always synchronous. There's a dumb hacky way to get around the issue, but I hesitate to suggest it, as it'll make the code nasty.

I can handle refactoring / fixing this specific issue, so please revert this change that you've made.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

} catch (e) {
cb(e);
return cb(e);
}
cb(err, zipTOC);
}

private static _computeIndexResponsive(data: Buffer, index: FileIndex<CentralDirectory>, cdPtr: number, cdEnd: number, cb: BFSCallback<ZipTOC>, cdEntries: CentralDirectory[], eocd: EndOfCentralDirectory) {
Expand Down
68 changes: 40 additions & 28 deletions src/core/file_system.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,10 +660,14 @@ export class BaseFileSystem {
} else if (encoding === null) {
return cb(err, buf);
}
// TODO: Share tryToString helper with HTTPRequest.ts
let res: string | undefined;
try {
cb(null, buf.toString(encoding));
res = buf.toString(encoding);
} catch (e) {
cb(e);
err = e;
} finally {
cb(err, res);
}
});
});
Expand Down Expand Up @@ -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<Stats>): 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<File>): 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<string[]>): 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<string>): 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);
}
}
7 changes: 5 additions & 2 deletions src/generic/key_value_filesystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = 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);
}
});
}
Expand Down
Loading