ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined #21440

ChALkeR · 2018-06-21T13:35:53Z

While taking a look at #21435 / #21421, I did a quick and dirty check which errors are being instantiated and /lib and checked what happened.

ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK seems to be used in loader, but never defined.

node/lib/internal/modules/esm/loader.js

Lines 6 to 8 in b56f65e

    
             ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, 
        
             ERR_UNKNOWN_MODULE_FORMAT 
        
           } = require('internal/errors').codes;

node/lib/internal/modules/esm/loader.js

Lines 95 to 96 in b56f65e

    
           if (typeof this._dynamicInstantiate !== 'function') 
        
             throw new ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK();

Not defined in lib/internal/errors.js.

ERR_TLS_RENEGOTIATE seems to be used in tls, but never defined.

node/lib/_tls_wrap.js

Lines 47 to 52 in b56f65e

    
             ERR_TLS_RENEGOTIATE, 
        
             ERR_TLS_RENEGOTIATION_DISABLED, 
        
             ERR_TLS_REQUIRED_SERVER_NAME, 
        
             ERR_TLS_SESSION_ATTACK, 
        
             ERR_TLS_SNI_FROM_SERVER 
        
           } = require('internal/errors').codes;

node/lib/_tls_wrap.js

Lines 572 to 574 in b56f65e

    
           if (callback) { 
        
             process.nextTick(callback, new ERR_TLS_RENEGOTIATE()); 
        
           }

Not defined in lib/internal/errors.js.

ERR_HTTP2_SETTINGS_CANCEL seems to be used in http2, but never defined.

node/lib/internal/http2/core.js

Lines 45 to 62 in b56f65e

    
               ERR_HTTP2_SETTINGS_CANCEL, 
        
               ERR_HTTP2_SOCKET_BOUND, 
        
               ERR_HTTP2_STATUS_101, 
        
               ERR_HTTP2_STATUS_INVALID, 
        
               ERR_HTTP2_STREAM_CANCEL, 
        
               ERR_HTTP2_STREAM_ERROR, 
        
               ERR_HTTP2_STREAM_SELF_DEPENDENCY, 
        
               ERR_HTTP2_TRAILERS_ALREADY_SENT, 
        
               ERR_HTTP2_TRAILERS_NOT_READY, 
        
               ERR_HTTP2_UNSUPPORTED_PROTOCOL, 
        
               ERR_INVALID_ARG_TYPE, 
        
               ERR_INVALID_CALLBACK, 
        
               ERR_INVALID_CHAR, 
        
               ERR_INVALID_OPT_VALUE, 
        
               ERR_OUT_OF_RANGE, 
        
               ERR_SOCKET_CLOSED 
        
             } 
        
           } = require('internal/errors');

node/lib/internal/http2/core.js

Lines 615 to 616 in b56f65e

    
           if (typeof cb === 'function') 
        
             cb(new ERR_HTTP2_SETTINGS_CANCEL());

Not defined in lib/internal/errors.js.

This probably affects all 10.x versions, but was not backported to earlier branches as is a semver-major.

Blame points at 1d2fd8b / #19137.

/cc @targos

This also probably needs a testcase when fixed, I do not have one atm.

The text was updated successfully, but these errors were encountered:

BridgeAR · 2018-06-21T14:06:17Z

@ChALkeR the blame is wrong. The typo was introduced in 921fb84. Because of the typo the error did not seem to be used anymore so I removed that in 6e1c25c.

ChALkeR · 2018-06-21T14:18:41Z

@BridgeAR I am looking only at JS side, not docs.

At 1d2fd8b, there is no ERR_MISSING_DY***, ERR_TLS_RENEGOTIATE or ERR_HTTP2_SETTINGS_CANCEL defined, but those are directly used as properties (for the first time).

I could be wrong with interpreting blame output, and those might have been removed earlier.

BridgeAR · 2018-06-21T14:24:10Z

Oh, I only looked at the first entry before the edits.

ChALkeR · 2018-06-21T14:43:51Z

This is the list of errors that seem to be missing documentation:

ERR_HTTP2_ERROR
ERR_UNKNOWN_BUILTIN_MODULE
ERR_HTTP2_SETTINGS_CANCEL
ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK
ERR_TLS_RENEGOTIATE

Those are the three mentioned above (that also miss implementation), plus:

ERR_HTTP2_ERROR (used in lib/internal/http2/util.js).
ERR_UNKNOWN_BUILTIN_MODULE (used in lib/internal/bootstrap/loaders.js).

BridgeAR · 2018-06-21T14:46:21Z

Was never properly ported: f67aa56 (the error code was not created at all).

BridgeAR · 2018-06-21T14:50:22Z

Was never properly implemented: bbaea12

BridgeAR · 2018-06-21T14:58:22Z

ERR_UNKNOWN_BUILTIN_MODULE is a false positive. It is actually added to the error directly instead of using internal/errors.

BridgeAR · 2018-06-21T15:00:04Z

ERR_HTTP2_ERROR is a false positive as well.

ChALkeR · 2018-06-21T15:08:10Z

@BridgeAR #21440 (comment) is specifically about documentation — I saw how those are created and it's ok that they are not present in internal/errors, but they should be documented nevertheless if the user might experience those. I do not think that those are false positives.

ChALkeR · 2018-06-21T15:11:21Z

Based on f67aa56, this might also affect v9.x branch.

BridgeAR · 2018-06-21T15:23:43Z

Oh, true. They were falsely removed in 1cdb41f.

ChALkeR · 2018-06-21T16:09:45Z

@BridgeAR Seems to be also a problem on v8.x, as the doc change was backported in #16556.
/cc @nodejs/lts

Also, @nodejs/collaborators, any idea how to prevent accidential removal of used error codes from the documentation in the future?

I can share my testcase.

ronkorving · 2018-06-22T01:12:12Z

This may be a good opportunity to rename ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK? :)

targos · 2018-06-22T12:05:11Z

git blame points to 1d2fd8b but that commit just moves things around. Things should have already been missing before it.

ChALkeR · 2018-06-22T13:39:04Z

Ok, I made the scanner aware of cpp-side errors, and found that ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER is also missing documentation.

To summarize:

Used, but missing implementation:

ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK
ERR_TLS_RENEGOTIATE
ERR_HTTP2_SETTINGS_CANCEL

Used, but missing documentation:

ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK
ERR_TLS_RENEGOTIATE
ERR_HTTP2_SETTINGS_CANCEL
ERR_HTTP2_ERROR
ERR_UNKNOWN_BUILTIN_MODULE
ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER

Documented, do not seem to be present anywhere:

ERR_STREAM_READ_NOT_IMPLEMENTED
ERR_VALUE_OUT_OF_RANGE

ChALkeR · 2018-06-22T14:08:23Z

Full output on v10.5.0:

js: ERR_TLS_RENEGOTIATE - missing JS implementation (source)
js: ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK - missing JS implementation (source)
js: ERR_HTTP2_SETTINGS_CANCEL - missing JS implementation (source)

init: ERR_METHOD_NOT_IMPLEMENTED - failed 0 args init, but is called with "()"

impl: ERR_STREAM_READ_NOT_IMPLEMENTED - documented, missing implementation
impl: ERR_VALUE_OUT_OF_RANGE - documented, missing implementation

doc: ERR_UNKNOWN_BUILTIN_MODULE - missing documentation
doc: ERR_TLS_RENEGOTIATE - missing documentation
doc: ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK - missing documentation
doc: ERR_HTTP2_SETTINGS_CANCEL - missing documentation
doc: ERR_HTTP2_ERROR - missing documentation
doc: ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER - missing documentation

Test script: https://gist.github.com/ChALkeR/a592f1e79624806b383007d7c5d4a395

ChALkeR · 2018-06-22T14:12:44Z

/cc @jasnell

ChALkeR · 2018-06-22T17:41:54Z

PR with my test script: #21470.

ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE error codes documentation seem to have been accidentally removed in commit 1cdb41f (pull request nodejs#15160). This reverts that removal, restoring the documentation for those two error codes. Those error codes are used from lib/ folder. This is a part of the fixes hinted by nodejs#21470, which includes some tests for error codes usage and documentation and enforces a stricter format. PR-URL: nodejs#21484 Refs: nodejs#21470 Refs: nodejs#21440 Refs: nodejs#15160 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE error codes documentation seem to have been accidentally removed in commit 1cdb41f (pull request #15160). This reverts that removal, restoring the documentation for those two error codes. Those error codes are used from lib/ folder. This is a part of the fixes hinted by #21470, which includes some tests for error codes usage and documentation and enforces a stricter format. PR-URL: #21484 Refs: #21470 Refs: #21440 Refs: #15160 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

thatshailesh · 2018-06-25T04:55:26Z

@ChALkeR Interested in picking this
Need help in using message text for ERR_TLS_RENEGOTIATE and ERR_HTTP2_SETTINGS_CANCEL

Is it fine to use messages below -
TLS session renegotiate failed for ERR_TLS_RENEGOTIATE
HTTP/2 pending settings has been canceled for ERR_HTTP2_SETTINGS_CANCEL

for the docs-
Attempt to regotiate tls session failed - ERR_TLS_RENEGOTIATE
Http2 session settings canceled - ERR_HTTP2_SETTINGS_CANCEL

Please let me know if there anything to add more here in docs or error message

davisjam · 2018-06-25T12:43:55Z

@thatshailesh Technically speaking I don't know the appropriate messages, but here's a quick spell check:

TLS session renegotiation failed
HTTP/2 pending settings have been canceled (Though this sounds a bit funny. Is the problem here that a settings request was cancelled?)

and then

Attempt to renegotiate TLS session failed
Http2 session settings canceled: Is this spelled Http2 or HTTP2?

thatshailesh · 2018-06-25T15:19:35Z

@davisjam Thanks for your answer
Actually settings frame MUST be sent by both endpoints at the start of a connection
so from what I understand is its been canceled while the session is waiting for the remote peer to acknowledge the new setting

its spelled HTTP2

ChALkeR · 2018-06-25T16:43:40Z

@thatshailesh Actually, I am not sure how to phrase those myself and do not have anything in mind off-hand, — that's one of the reasons why I preferred to delegate it to someone. So I am not the best choice to ask about phrasing. 😃

I would say — open a pull request with some phrasing that seems fine to you, ask for review from involved people, e.g. those who introduced that API and other collaborators, and see how it goes. 😉

thatshailesh · 2018-06-25T17:17:51Z

Ok Sure 👍

Working on it

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR nodejs#16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR nodejs#18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: nodejs#21440 Refs: nodejs#21470 Refs: nodejs#16874 Refs: nodejs#18857

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: #21440 Refs: #21470 Refs: #16874 Refs: #18857 PR-URL: #21493 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Includes implementation of tls, HTTP2 error with documentation. PR-URL: #21564 Refs: #21440 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

ChALkeR · 2018-07-23T15:50:11Z

I believe this was fixed in #21493 and #21564.

Includes implementation of tls, HTTP2 error with documentation. PR-URL: nodejs#21564 Refs: nodejs#21440 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

This adds several sanity checks for error codes. It scans: * all natives (js sources), * doc/api/*.md documentation * src/node_errors.h (errors definition from the C++ side). There is also a whitelist of manually created errors from JS side, currently consisting of ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE. Alsom all ERR_NAPI_ codes are whitelisted, as those are created directly on the cpp side, without declaring them first. The performed checks: 1. All errors used from JS should be defined in `internal/errors` and present in its .codes object. Whitelist (mentioned above) applies. 2. All errors instantiated from JS without arguments should support 0-arguments version. 3. All errors mentioned in doc should defined either in JS, C++, or in the whitelist. 4. All errors mentioned anywhere should be documented. 5. Documentation of error codes should be sorted, have no repeats, and include exactly one entry for every error code mentioned in the documentation, formatted as `/\n### (ERR_[A-Z0-9_]+)\n`. 6. All doc entries for error codes should have appropriate anchors. There is also a --report flag, which prints all the current issues and exits without asserting, for manual inspection. Individual fixes for those issues are landed in separate commits. Refs: nodejs#21421 Refs: nodejs#21440 Refs: nodejs#21483 Refs: nodejs#21484 Refs: nodejs#21485 Refs: nodejs#21487 PR-URL: nodejs#21470

Includes implementation of tls, HTTP2 error with documentation. PR-URL: nodejs#21564 Refs: nodejs#21440 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Includes implementation of tls, HTTP2 error with documentation. Backport-PR-URL: #22850 PR-URL: #21564 Refs: #21440 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

ChALkeR added errors Issues and PRs related to JavaScript errors originated in Node.js core. tls Issues and PRs related to the tls subsystem. http2 Issues or PRs related to the http2 subsystem. labels Jun 21, 2018

ChALkeR changed the title ~~ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK is not defined~~ ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined Jun 21, 2018

This was referenced Jun 21, 2018

doc: document missing error codes #15160

Closed

[v8.x backport] doc: document missing error codes #16556

Closed

ChALkeR added lib / src Issues and PRs related to general changes in the lib or src directory. doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support. labels Jun 22, 2018

ChALkeR mentioned this issue Jun 22, 2018

test: add test for error codes doc and impl #21470

Closed

3 tasks

ChALkeR mentioned this issue Jun 23, 2018

doc: add a missing anchor to error codes #21483

Merged

4 tasks

ChALkeR changed the title ~~ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined~~ ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined Jun 24, 2018

thatshailesh mentioned this issue Jun 27, 2018

errors: fix undefined HTTP2 and tls errors #21564

Closed

4 tasks

ChALkeR mentioned this issue Jun 30, 2018

Discussion: Exit criteria for bringing http2 out of experimental #19453

Closed

5 tasks

ChALkeR closed this as completed Jul 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined #21440

ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined #21440

ChALkeR commented Jun 21, 2018 •

edited

Loading

BridgeAR commented Jun 21, 2018

ChALkeR commented Jun 21, 2018 •

edited

Loading

BridgeAR commented Jun 21, 2018

ChALkeR commented Jun 21, 2018 •

edited

Loading

BridgeAR commented Jun 21, 2018

BridgeAR commented Jun 21, 2018

BridgeAR commented Jun 21, 2018

BridgeAR commented Jun 21, 2018

ChALkeR commented Jun 21, 2018 •

edited

Loading

ChALkeR commented Jun 21, 2018

BridgeAR commented Jun 21, 2018

ChALkeR commented Jun 21, 2018 •

edited

Loading

ronkorving commented Jun 22, 2018

targos commented Jun 22, 2018

ChALkeR commented Jun 22, 2018 •

edited

Loading

ChALkeR commented Jun 22, 2018 •

edited

Loading

ChALkeR commented Jun 22, 2018

ChALkeR commented Jun 22, 2018

thatshailesh commented Jun 25, 2018

davisjam commented Jun 25, 2018

thatshailesh commented Jun 25, 2018

ChALkeR commented Jun 25, 2018 •

edited

Loading

thatshailesh commented Jun 25, 2018

ChALkeR commented Jul 23, 2018 •

edited

Loading

ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined #21440

ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined #21440

Comments

ChALkeR commented Jun 21, 2018 • edited Loading

BridgeAR commented Jun 21, 2018

ChALkeR commented Jun 21, 2018 • edited Loading

BridgeAR commented Jun 21, 2018

ChALkeR commented Jun 21, 2018 • edited Loading

BridgeAR commented Jun 21, 2018

BridgeAR commented Jun 21, 2018

BridgeAR commented Jun 21, 2018

BridgeAR commented Jun 21, 2018

ChALkeR commented Jun 21, 2018 • edited Loading

ChALkeR commented Jun 21, 2018

BridgeAR commented Jun 21, 2018

ChALkeR commented Jun 21, 2018 • edited Loading

ronkorving commented Jun 22, 2018

targos commented Jun 22, 2018

ChALkeR commented Jun 22, 2018 • edited Loading

Used, but missing implementation:

Used, but missing documentation:

Documented, do not seem to be present anywhere:

ChALkeR commented Jun 22, 2018 • edited Loading

ChALkeR commented Jun 22, 2018

ChALkeR commented Jun 22, 2018

thatshailesh commented Jun 25, 2018

davisjam commented Jun 25, 2018

thatshailesh commented Jun 25, 2018

ChALkeR commented Jun 25, 2018 • edited Loading

thatshailesh commented Jun 25, 2018

ChALkeR commented Jul 23, 2018 • edited Loading

ChALkeR commented Jun 21, 2018 •

edited

Loading

ChALkeR commented Jun 21, 2018 •

edited

Loading

ChALkeR commented Jun 21, 2018 •

edited

Loading

ChALkeR commented Jun 21, 2018 •

edited

Loading

ChALkeR commented Jun 21, 2018 •

edited

Loading

ChALkeR commented Jun 22, 2018 •

edited

Loading

ChALkeR commented Jun 22, 2018 •

edited

Loading

ChALkeR commented Jun 25, 2018 •

edited

Loading

ChALkeR commented Jul 23, 2018 •

edited

Loading