Skip to content

Commit

Permalink
fs: avoid circular dependency in SyncWriteStream
Browse files Browse the repository at this point in the history
Previously, there was a circular dependency on the public fs module in
SyncWriteStream. Moving the implementations of fs.writeSync and
fs.closeSync to internal/fs allows us to avoid that circular dependency.

Fixes: nodejs#11257
  • Loading branch information
evanlucas committed Mar 23, 2017
1 parent b5eccc4 commit fdcfe99
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 23 deletions.
23 changes: 4 additions & 19 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ const internalURL = require('internal/url');
const internalUtil = require('internal/util');
const assertEncoding = internalFS.assertEncoding;
const stringToFlags = internalFS.stringToFlags;
const writeSync = internalFS.writeSync;
const closeSync = internalFS.closeSync;
const getPathFromURL = internalURL.getPathFromURL;
const { StorageObject } = require('internal/querystring');

Expand Down Expand Up @@ -600,9 +602,7 @@ fs.close = function(fd, callback) {
binding.close(fd, req);
};

fs.closeSync = function(fd) {
return binding.close(fd);
};
fs.closeSync = closeSync;

function modeNum(m, def) {
if (typeof m === 'number')
Expand Down Expand Up @@ -710,22 +710,7 @@ fs.write = function(fd, buffer, offset, length, position, callback) {
// fs.writeSync(fd, buffer[, offset[, length[, position]]]);
// OR
// fs.writeSync(fd, string[, position[, encoding]]);
fs.writeSync = function(fd, buffer, offset, length, position) {
if (isUint8Array(buffer)) {
if (position === undefined)
position = null;
if (typeof offset !== 'number')
offset = 0;
if (typeof length !== 'number')
length = buffer.length - offset;
return binding.writeBuffer(fd, buffer, offset, length, position);
}
if (typeof buffer !== 'string')
buffer += '';
if (offset === undefined)
offset = null;
return binding.writeString(fd, buffer, offset, length, position);
};
fs.writeSync = writeSync;

fs.rename = function(oldPath, newPath, callback) {
callback = makeCallback(callback);
Expand Down
32 changes: 28 additions & 4 deletions lib/internal/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

const Buffer = require('buffer').Buffer;
const Writable = require('stream').Writable;
const fs = require('fs');
const util = require('util');
const constants = process.binding('constants').fs;
const binding = process.binding('fs');
const { isUint8Array } = process.binding('util');

const O_APPEND = constants.O_APPEND | 0;
const O_CREAT = constants.O_CREAT | 0;
Expand Down Expand Up @@ -54,6 +55,27 @@ function stringToFlags(flag) {
throw new Error('Unknown file open flag: ' + flag);
}

function writeSync(fd, buffer, offset, length, position) {
if (isUint8Array(buffer)) {
if (position === undefined)
position = null;
if (typeof offset !== 'number')
offset = 0;
if (typeof length !== 'number')
length = buffer.length - offset;
return binding.writeBuffer(fd, buffer, offset, length, position);
}
if (typeof buffer !== 'string')
buffer += '';
if (offset === undefined)
offset = null;
return binding.writeString(fd, buffer, offset, length, position);
}

function closeSync(fd) {
return binding.close(fd);
}

// Temporary hack for process.stdout and process.stderr when piped to files.
function SyncWriteStream(fd, options) {
Writable.call(this);
Expand All @@ -70,7 +92,7 @@ function SyncWriteStream(fd, options) {
util.inherits(SyncWriteStream, Writable);

SyncWriteStream.prototype._write = function(chunk, encoding, cb) {
fs.writeSync(this.fd, chunk, 0, chunk.length);
writeSync(this.fd, chunk, 0, chunk.length);
cb();
return true;
};
Expand All @@ -80,7 +102,7 @@ SyncWriteStream.prototype._destroy = function() {
return;

if (this.autoClose)
fs.closeSync(this.fd);
closeSync(this.fd);

this.fd = null;
return true;
Expand All @@ -97,5 +119,7 @@ module.exports = {
assertEncoding,
stringToFlags,
SyncWriteStream,
realpathCacheKey: Symbol('realpathCacheKey')
realpathCacheKey: Symbol('realpathCacheKey'),
writeSync,
closeSync
};
27 changes: 27 additions & 0 deletions test/parallel/test-stdin-require.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const execSync = require('child_process').execSync;
const fs = require('fs');
const path = require('path');

// Prevents a regression where redirecting stderr and receiving input script
// via stdin works properly.

common.refreshTmpDir();
const filename = path.join(common.fixturesDir, 'baz.js');
const out = path.join(common.tmpDir, 'js.out');
const bin = process.execPath;
const input = `require('${filename}'); console.log('PASS');`;
const cmd = common.isWindows ?
`echo "${input}" | ${bin} > ${out} 2>&1` :
`echo "${input}" | ${bin} &> ${out}`;

// This will throw if internal/fs has a circular dependency.
assert.strictEqual(execSync(cmd).toString(), '');

const result = fs.readFileSync(out, 'utf8').trim();
assert.strictEqual(result, 'PASS');

fs.unlinkSync(out);

0 comments on commit fdcfe99

Please sign in to comment.