fs: avoid circular dependency in SyncWriteStream

[evanlucas]

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.

This is an alternative to #11260.

Fixes #11257 (added by @BridgeAR)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

fs

[Fishrock123]

CI: https://ci.nodejs.org/job/node-test-pull-request/6975/

[evanlucas]

so the first CI run failed on some of the windows boxes. Do they not all have cat?

[bzoz]

Windows shell does not support using ; to separate commands. The test needs to be rewritten for windows.

[gibfahn]

@bzoz could you use a newline instead of ;?

[bzoz]

@gibfahn that would not work also. From the first glance it looks like cat is used to read file content. This can be done by node itself.

[gibfahn]

Yeah okay, so @evanlucas looks like the choices are:

1. Run two separate commands, e.g.

const cmd = `echo "${input}" | ${bin} > ${out} 2>&1`;
const cmd2 = `cat ${out}`;
assert.strictEqual(execSync(cmd).toString(), '');
const result = execSync(cmd2).toString();

2. use fs.readFileSync() or similar and do the read in node.

I assume there's a reason to do the read not through node?

Also if you could add a brief comment to the top of the test explaining what it tests that would be really helpful. It's not immediately obvious to me what it checks.

[richardlau]

common.fixturesDir?

[evanlucas]

Ok, updated. PTAL

I assume there's a reason to do the read not through node?

No, there wasn't. This used the same script from the original test case. I switched to just reading the file via node. Thanks for checking me on that :]

I also switched to using common.fixturesDir

CI: https://ci.nodejs.org/job/node-test-pull-request/6998/

[evanlucas]

ouch, CI didn't seem to like that much on anything but OS X :[. I'll dig more into this.

[BridgeAR]

@evanlucas would you mind looking into this again? I think it would be great if we can get this fix in.

[BridgeAR]

Ping @evanlucas

[evanlucas]

I kept running in to trouble getting a test that works across all platforms. I don't really have the time to pick this up right now (I'm about to go on vacation), so if someone else wants to pick up here, fine by me. Otherwise, feel free to close and I'll try to get around to it when I get back. Thanks!

[BridgeAR]

Ping @nodejs/platform-windows @refack would someone mind to take a look at the windows failures?

[bzoz]

The command for Windows should have extra quotes

`echo "${input}" | "${bin}" > "${out}" 2>&1`

That said, this test passes on my Node 8.5.0. Is this PR still needed?

[BridgeAR]

@bzoz I can not reproduce this either anymore but in the original bug report there is a user that reported this for 8.5 as well.

[BridgeAR]

I am closing this as it seems to be resolved in v8.6 even though it is not clear what really solved it.