tls: Add support for ALPN fallback when no ALPN protocol matches#45075
tls: Add support for ALPN fallback when no ALPN protocol matches#45075pimterry wants to merge 1 commit intonodejs:mainfrom
Conversation
|
Review requested:
|
pimterry
force-pushed
the
alpn-wildcard
branch
from
c906151 to
de6f3d5
Compare
nodejs-github-bot
added
c++
pimterry
force-pushed
the
alpn-wildcard
branch
from
de6f3d5 to
eccdfd0
Compare
|
This PR conflicts with #44875 (sorry, I thought it'd been merged) and since that PR optimizes the common case and this PR adds an uncommon case, I'm going to have to ask you to rebase on top of that. I would have asked you to take another approach anyway because |
| protocol name. Passing an array is usually much simpler, e.g. | ||
| `['hello', 'world']`. (Protocols should be ordered by their priority.) | ||
| * `allowALPNFallback`: {boolean} If `true`, if the `ALPNProtocols` option was | ||
| set and the client uses ALPN, but requests only protocols that are not do |
There was a problem hiding this comment.
Awkwardly structured sentence / grammar here.
| ->Get(env->context(), env->allow_alpn_fallback_string()) | ||
| .ToLocalChecked() | ||
| .As<Boolean>() | ||
| ->Value(); |
There was a problem hiding this comment.
As @bnoordhuis mentions, this is going to be slow. Definitely should avoid uses of ->Get(...)
There was a problem hiding this comment.
I'm not very familiar with the JS/C++ interface. What's the preferred way to set a value from the JS TLS wrap such that it's directly accessible here?
If I create a new private field & setter on the C++ wrap, call that from JS in setup, and then read w->allow_alpn_fallback here instead of these calls, is that a) idiomatic and b) sufficiently performant?
|
Happy to do the final rebase & changes here, but I'm going to wait for a conclusion on the the callback question in #45056 (comment) first, since that would invalidate this. |
4 participants
Fixes #45056.