Skip to content

Replace synchronous file operations with async versions #42

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Replace synchronous file operations with async versions #42

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a8dcb8
Replace fs sync operations with async versions
catzhang Jul 25, 2020
b36b915
Merge branch 'master' of https://github.com/reberhardt7/cplayground i…
catzhang Jul 25, 2020
2535f14
Fix lint errors
catzhang Jul 25, 2020
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
64 changes: 38 additions & 26 deletions src/server/container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,21 @@ export default class Container {
private saveCodeFiles = (code: string, includeFileData: Buffer | null): void => {
// Create data directory and save code from request
console.log(`${this.logPrefix}Saving code to ${this.codeHostPath}`);
if (!fs.existsSync(this.dataHostPath)) fs.mkdirSync(this.dataHostPath);
fs.writeFileSync(this.codeHostPath, code);
fs.access(this.dataHostPath, fs.constants.F_OK, (errPathExists) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rename errPathExists to something like accessError, since the "PathExists" part could imply that the path does exist

if (errPathExists) {
fs.mkdir(this.dataHostPath, (err) => { if (err) throw err; });
}
});
fs.writeFile(this.codeHostPath, code, (err) => {
Copy link
Owner

Choose a reason for hiding this comment

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

The way that Javascript does concurrency can be super confusing at first. We want to do this sequentially:

  • Check if the directory exists
  • Create it if it doesn't
  • Write the file

It might look like this code does that, but actually it does this:

  • Check if the directory exists
    • Whenever the check completes, create it if it doesn't
  • Immediately proceed to write the file

If we could only use callbacks, we would need to put all code following fs.access inside the callback function. This gets ugly pretty quickly. Instead, this codebase uses promises with async/await syntax. The fs.promises module contains a bunch of versions of fs calls that return promises instead of executing callbacks.

We should be able to do something like this:

try {
    await fs.promises.mkdir(this.dataHostPath);
} catch (err) {
    // Ignore the error if the directory already exists. Otherwise, re-throw it
    if (err && err.code != 'EEXIST') {
        throw err;
    }
}
await fs.promises.writeFile(this.codeHostPath, code);
...

The fs.promises calls return a promise, which is an object that indicates whether the operation has finished (and any returned values or errors from the underlying callback). Then, await says to wait until the promise resolves before executing the subsequent code. (It's actually quite a bit more involved under the hood, but this is the basic idea.)

You'll also need to convert saveCodeFiles to be an async function by adding the async keyword:

private saveCodeFiles = async (code: string, includeFileData: Buffer | null): void => ...

Now that saveCodeFiles is an async function, it returns a promise right away (before it finishes executing) instead of executing synchronously. That means you'll need to add await everywhere it's called.

It looks like it gets called from the Container constructor. Unfortunately constructors cannot be async functions (Javascript requires that objects are constructed synchronously), so you won't be able to use await there. However, since async just makes a function return an ordinary promise, you can do something like this:

this.saveCodeFiles(code, includeFileData)
    .then(enableDebugging ? this.initializeDebugSocket : Promise.resolve))
    .then(() => this.startContainer(compiler, debugAwareCflags, argsStr, rows, cols, enableDebugging));

Or, since I think we're starting to get several async-y things that we need to do in the constructor, we could define an async function inside of the constructor and invoke that:

// towards end of constructor
(async (): void => {
    await this.saveCodeFiles(code, includeFileData);
    if (enableDebugging) {
        await this.initializeDebugSocket();
    }
    await this.startContainer(.....);
})().then(); // the .then() is just to suppress a warning about ignoring the promise returned by the async function

if (err) throw err;
console.log(`${this.logPrefix}Saved file to ${this.codeHostPath}`);
});
if (includeFileData) {
console.log(`${this.logPrefix}Writing include file to ${this.includeFileHostPath}`);
fs.writeFileSync(this.includeFileHostPath, includeFileData);
fs.writeFile(this.includeFileHostPath, includeFileData, (err) => {
if (err) throw err;
console.log(`${this.logPrefix}Saved file to ${this.includeFileHostPath}`);
});
}
};

Expand Down Expand Up @@ -315,8 +325,12 @@ export default class Container {
// Remove uploaded file. We don't care about errors, in case the file
// was already removed (or was never successfully created to begin
// with)
try { fs.unlinkSync(this.codeHostPath); } catch { /* ignore */ }
try { fs.unlinkSync(this.includeFileHostPath); } catch { /* ignore */ }
fs.unlink(this.codeHostPath, () => {
console.log(`${this.logPrefix}Deleted ${this.codeHostPath}`);
});
fs.unlink(this.includeFileHostPath, () => {
console.log(`${this.logPrefix}Deleted ${this.includeFileHostPath}`);
});
// Shut down gdb server, if it's running
if (this.gdbMiServer) {
this.gdbMiServer.close();
Expand Down Expand Up @@ -481,27 +495,25 @@ export default class Container {
// have started yet, and there's not much we can do
if (!this.containerId) return;

let cpuUsageNs;
try {
// TODO: get rid of synchronous read
cpuUsageNs = parseInt(
fs.readFileSync(
`/sys/fs/cgroup/cpu/docker/${this.containerId}/cpuacct.usage`,
).toString(),
10,
);
} catch (exc) {
console.warn(`${this.logPrefix}Error loading cgroup CPU usage!`, exc);
return;
}
const cpuUsageMs = cpuUsageNs / 1000000;
console.debug(`${this.logPrefix}Current CPU time used: ${cpuUsageMs}ms`);
if (cpuUsageMs > MAX_CPU_TIME) {
console.warn(`${this.logPrefix}Container ${this.containerName} exceeded its CPU `
+ 'quota! Killing container');
childProcess.execFile('docker', ['kill', this.containerName]);
this.showErrorBanner('The program exceeded its CPU quota.');
}
fs.readFile(
Copy link
Owner

Choose a reason for hiding this comment

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

Same comments here about using async/await with promises instead of callbacks

`/sys/fs/cgroup/cpu/docker/${this.containerId}/cpuacct.usage`,
(err, data) => {
if (err) {
console.warn(`${this.logPrefix}Error loading cgroup CPU usage!`, err);
return;
}

const cpuUsageNs = parseInt(data.toString(), 10);
const cpuUsageMs = cpuUsageNs / 1000000;
console.debug(`${this.logPrefix}Current CPU time used: ${cpuUsageMs}ms`);
if (cpuUsageMs > MAX_CPU_TIME) {
console.warn(`${this.logPrefix}Container ${this.containerName} exceeded its CPU `
+ 'quota! Killing container');
childProcess.execFile('docker', ['kill', this.containerName]);
this.showErrorBanner('The program exceeded its CPU quota.');
}
},
);
}, 1000);
};

Expand Down