dns: return real system error instead of 'ENOTFOUND'

[ncopa]

Return the real system error to user insetad of injecting 'ENOTFOUND'
which is not a proper POSIX error.

#5099 (comment)

Also fix the test suite to check for the corresponding error message.

The hostname '...' was replaced with '***' because '...' returns
inconsistent error message on different system. On GNU libc it intantly
returns EAI_NONAME but on musl libc it returns EAI_AGAIN after a
timeout.

[silverwind]

I'm not sure we can accept this breaking change. The current plan is to return real DNS error codes like NXDOMAIN and the change is part of an upcoming rewrite of the dns module.

cc: @mscdex

[mscdex]

Like @silverwind said, this would definitely be a breaking change. I do not mind if this gets landed in the interim, before #1843 (if it does land), as that PR could still be awhile yet.

[silverwind]

@mscdex are you in support of these system error codes? I'd say if we're going to mess with these, we should just do DNS error codes, if the c-ares codes can all be mapped to DNS errors (which I doubt).

[mscdex]

@silverwind I'm not sure that the actual DNS status codes are available? I haven't really looked into it though. There will probably still need to be non-DNS error names though for problems that arise that don't fit any of the DNS status codes.

[Trott]

What's the reason for changing ... to ***? (Sorry if it's a naive question.)

[ncopa]

@Trott as I tried to explain in the commit message, the getaddrinfo() error code when hostname is ... is different on different systems:

libc	hostname	getaddrinfo() error code
glibc	...	EAI_NONAME
musl	...	EAI_AGAIN
glibc	***	EAI_NONAME
musl	***	EAI_NONAME

So I changed the hostname instead of testing for both error codes:

 c.on('error', common.mustCall(function(e) {
-  assert.equal(e.code, 'ENOTFOUND');
+  assert(e.code == 'EAI_NONAME' || e.code == 'EAI_AGAIN');
   assert.equal(e.port, common.PORT);

Also, on musl using ... would make the test to wait for a timeoutm while using *** would fail instantly.

[ncopa]

I am open for suggestions for a better commit message.

[Trott]

@ncopa Totally my fault for not reading the commit message before commenting!

[jasnell]

@nodejs/ctc ... any thoughts on this one?
@ncopa ... can you rebase and update this?

[Trott]

Maybe @ChALkeR, someone at npm, or someone else who is clever can give us an idea of how often ENOTFOUND shows up in npm modules. I suspect a ton of ecosystem breakage from this change but would be delighted to be wrong. A search on GitHub is certainly discouraging...

[rvagg]

IMO this probably has too big an impact to even do on a semver-major, the ship might have sailed on this. Perhaps a compromise would be to use the original code as an additional property on the err, maybe something like:

function errnoException(err, syscall, hostname) {
  var eai = null;
  if (err === uv.UV_EAI_MEMORY ||
      err === uv.UV_EAI_NODATA ||
      err === uv.UV_EAI_NONAME) {
    eai = uv.errname(err);
    err = 'ENOTFOUND';
  }
  var ex = null;
  if (typeof err === 'string') {  // c-ares error code.
    ex = new Error(syscall + ' ' + err + (hostname ? ' ' + hostname : ''));
    ex.code = err;
    ex.errno = err;
    ex.syscall = syscall;
    ex.eai = eai;
  } else {
    ex = util._errnoException(err, syscall);
  }
  if (hostname) {
    ex.hostname = hostname;
  }
  return ex;
}

@bnoordhuis what are your thoughts here since it's your TODO being addressed?

[jasnell]

One possibility: we could document a pending deprecation of ENOTFOUND in v6 and make this change in v7

[jbergstroem]

I'm +1 to @jasnell's suggestion. Exposing the system error is the correct thing to do and we should strive to do just that. We could perhaps introduce some compat/npm library for people that does't want to change their codebase in v7.

[jasnell]

@bnoordhuis @nodejs/ctc ... thoughts on this? Specifically my proposal here: #5196 (comment)

[trevnorris]

@jasnell I'm not sure we can "deprecate" ENOTFOUND since there's no overlap of an alternative API. This is a hard swap. So it'd be more an impending change.

[Fishrock123]

Is there a way we could append an additional property specifically for these errors? That is the only option that comes to mind other than a hard switch...

[jasnell]

@trevnorris ... yes, by "deprecation" I mean only giving users a clear indication that a hard swap would be made after v7 ... not a deprecation in the traditional sense with a printed warning.

[ChALkeR]

@Trott https://gist.github.com/ChALkeR/b720b21c377809e0eb2796aa5df42bfa — the usage is quite low.
Sorry for the delay, I somehow missed this.

Update: sorted the list by downloads/month.

[rvagg]

We have Alpine in CI at the moment (won't stay if we can't get this sorted out), the EAI_AGAIN errors are the only ones left standing: https://ci.nodejs.org/job/node-test-commit-linux/3804/nodes=ubuntu1604_docker_alpine34-64

[ChALkeR]

Related: #1011.
Also, I sorted the grep above by downloads/month.

/cc @othiym23 for npm.

[silverwind]

Hmm I think I'm -1 on this at this point. Different errors for different libc implementations seems like a unneccesary detail we shouldn't expose our users to. I see some value in having real DNS errors like NXDomain (https://tools.ietf.org/html/rfc6895), though.

[ncopa]

I think this actually is a bug in musl libc. I sent a patch for fixing it. I will follow that up.

[jasnell]

@nodejs/ctc ... and @bnoordhuis in particular... is this something we'd actually want to do at this point?

[cjihrig]

We could just attach the real error to the existing error, right?

[jasnell]

yep.. we don't have an existing property we can use but we can certainly make one.

[silverwind]

I could live with a err.status property that contains the DNS error strings like NXDOMAIN. This would be similar to how dig presents it.

[fhinkel]

ping @ncopa

[jasnell]

Closing due to lack of further activity.