lib: add chainErrors API to avoid error swallowing

[aduh95]

I think the current behaviour is wrong:

• it waits for the file to be closed, but there's no need for this, the error could be passed directly to the user.
• It swallows the error on closing. Failing to close a file is fairly rare in practice, but I think it would make sense to use the default behaviour added in fs: use a default callback for fs.close() #37174 (rethrow the error if one occurs).

Conservatively marking this as semver-major as it might raise an error where it didn't before.
EDIT: I've repurposed this PR to aggregate errors instead.

/cc @nodejs/fs

[RaisinTen]

The current behaviour is indeed incorrect.
This was introduced in this commit: ef55324
Before the commit, it worked just like your patch:

node/src/node.js

Lines 621 to 625 in 662fdfe

	fs.write(fd, _data, 0, encoding)
	.addErrback(function () {
	fs.close(fd);
	promise.emitError();
	})

[ronag]

I think this is better?

      if (isUserFd) {
        callback(writeErr);
      } else {
        fs.close(fd, function close(er) {
          callback(err || writeErr);
        });
      }

[aduh95]

I think this is better?

      if (isUserFd) {
        callback(writeErr);
      } else {
        fs.close(fd, function close(er) {
          callback(err || writeErr);
        });
      }

This would swallow the writeErr, which is worse than the current behavior IMHO.

[ronag]

This would swallow the writeErr, which is worse than the current behavior IMHO.

Swallow how? Only if fs.close fails (which is very unlikely). We always propagate the most inner failure.

[aduh95]

Swallow how? Only if fs.close fails (which is very unlikely). We always propagate the most inner failure.

You're right, I think I misread your suggestion at first, sorry.

Why do you think it is better? Is it because it's more consistent with other fs functions?
I'd say it's not useful to wait for the file to close to give the error back to the user, as they would more likely care for a write error than a close error (which is probably why the current way has been implemented in the first place).

[ronag]

Why do you think it is better? Is it because it's more consistent with other fs functions?
I'd say it's not useful to wait for the file to close to give the error back to the user, as they would more likely care for a write error than a close error (which is probably why the current way has been implemented in the first place).

Consistency. We always propagate innermost error.

[aduh95]

Hum it seems we are consistently ignoring error on close, and prefer report the other error:

node/lib/fs.js

Lines 1286 to 1292 in ffa874f

	// Prefer to return the chmod error, if one occurs,
	// but still try to close, and report closing errors if they occur.
	fs.fchmod(fd, mode, (err) => {
	fs.close(fd, (err2) => {
	callback(err || err2);
	});
	});

node/lib/fs.js

Lines 822 to 824 in ffa874f

	fs.close(fd, (er2) => {
	callback(er || er2);
	});

I think that makes sense as the close error is less interesting when you are performing a write operation.

[BridgeAR]

@ronag

Consistency. We always propagate innermost error.

This makes debugging difficult. Instead, we should always propagate the most outer error. The inner error is likely an error that is related to the former one without containing the most important information.

We could combine the errors by attaching inner errors to the outer one.

[ronag]

This makes debugging difficult. Instead, we should always propagate the most outer error. The inner error is likely an error that is related to the former one without containing the most important information.

This is not how we do it in a lot of other places. Whether we propagate the inner or outer error is a bigger discussion I think we should have (@mcollina, @lpinca, @jasnell, @addaleax) maybe in a separate issue? FYI I prefer propagating the inner most error.

My current understanding is that we always (should) propagate the inner error. Regardless whether that is the best thing or not, I think it's important we are consistent, i.e. propagate the inner error for now until we have a different consensus and then maybe change it in a semver-major.

[aduh95]

Chaining errors would work great as long as we are able to generate comprehensible stack traces. Something like a causedBy property on the error object, maybe?

[mcollina]

This needs a test.

[aduh95]

This needs a test.

@mcollina Not sure how to test this as it only occurs if both write and close operations fail. Can you guide me on how to make fs.close reliably fail?

[mcollina]

Maybe @jasnell have some ideas?

[ronag]

Monkey patch fs?

[aduh95]

I've taken the suggestion from BridgeAR to chain errors instead of trying to guess which one should be reported to the user. I've also added tests, PTAL.

[mcollina]

After thinking about this for a while, I'm starting to think we should handle the .code property somehow. This is going to break almost all error handling cases that checked .code, making it significantly more complex and error prone.

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

Landed in 104dac7