tls_wrap: migrate synchronous errors#18125
tls_wrap: migrate synchronous errors#18125joyeecheung wants to merge 3 commits intonodejs:masterfrom
Conversation
fc0aaf8 to
2429113
Compare
|
cc @jasnell @nodejs/tsc |
lib/_tls_wrap.js
Outdated
There was a problem hiding this comment.
It just needs to be a StreamBase, even if LibuvStreamWrap is the common case here.
There was a problem hiding this comment.
@addaleax I thought about checking the type of handle here but there doesn't seem to be a way of checking those in JS land since there isn't a proper inheritance of StreamBase.
There was a problem hiding this comment.
@joyeecheung Yeah, externalStream doesn’t really tell you what kind of thing it is. :/
Ideally, what we could do is making the StreamBase inheritance hierarchy reflected in the actual JS handle classes; that requires more work than this PR, though…
There was a problem hiding this comment.
@addaleax Yeah that should probably better be done in a dedicated PR IMO
lib/_tls_wrap.js
Outdated
There was a problem hiding this comment.
Should these be throw errors.TypeErrors instead of asserts?
There was a problem hiding this comment.
@maclover7 I tried that before and the test looked really dubious. These are not user-facing errors, whoever hits then either is using the underscored API or patching internals incorrectly, or is hitting a bug. Either way assertions seem to be more appropriate.
|
LGTM
…On Fri, Jan 12, 2018 at 7:46 PM Joyee Cheung ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/_tls_wrap.js
<#18125 (comment)>:
> @@ -407,7 +410,12 @@ TLSSocket.prototype._wrapHandle = function(wrap) {
const context = options.secureContext ||
options.credentials ||
tls.createSecureContext(options);
- const res = tls_wrap.wrap(handle._externalStream,
+ const externalStream = handle._externalStream;
+ assert(typeof externalStream === 'object',
@maclover7 <https://github.com/maclover7> I tried that before and the
test looked really dubious. These are not user-facing errors, whoever hits
then either is using the underscored API or patching internals incorrectly,
or is hitting a bug. Either way assertions seem to be more appropriate.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#18125 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAOjw2op6S37uxsZzpAEi3QydHUT1Sjvks5tJ_zlgaJpZM4RczA6>
.
|
* Throw ERR_INVALID_ARG_TYPE from public APIs * Assert argument types in bindings instead of throwing errors PR-URL: nodejs#18125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
* Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a server-side socket instead of returning silently * Assert on wrap_->started and wrap_->ssl instead of throwing errors since these errors indicate that the user either uses private APIs, or monkey-patches internals, or hits a bug. PR-URL: nodejs#18125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
2429113 to
20c9a40
Compare
20c9a40 to
4554a4b
Compare
|
Updated since #17882 has landed and introduced a check on a changed error. The assertion is probably equally unhelpful as before but since New CI: https://ci.nodejs.org/job/node-test-pull-request/12537/ |
|
Landed in f75bc2c...1c29da8, thanks! |
* Throw ERR_INVALID_ARG_TYPE from public APIs * Assert argument types in bindings instead of throwing errors PR-URL: #18125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
* Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a server-side socket instead of returning silently * Assert on wrap_->started and wrap_->ssl instead of throwing errors since these errors indicate that the user either uses private APIs, or monkey-patches internals, or hits a bug. PR-URL: #18125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
tls_wrap: migrate C++ errors to internal/errors.js
server-side socket instead of returning silently
errors since these errors indicate that the user either uses
private APIs, or monkey-patches internals, or hits a bug.
tls_wrap: migrate argument type-checking errors
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
tls, errors