-
Notifications
You must be signed in to change notification settings - Fork 5.7k
fix(ext/node): support JS underlying stream in TLS #30465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
disallowed-types = [ | ||
{ path = "std::sync::Arc", reason = "use deno_fs::sync::MaybeArc instead" }, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the lint rule doesn't help with anything. https://deno-company.slack.com/archives/C03P8NFAJTY/p1755693378481979
let (network_to_tls_tx, network_to_tls_rx) = | ||
tokio::sync::mpsc::channel::<Bytes>(10); | ||
let (tls_to_network_tx, tls_to_network_rx) = | ||
tokio::sync::mpsc::channel::<Bytes>(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these totally random buffer sizes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would never have more than 1 message in queue though.
let tls_stream = TlsStream::new_client_side( | ||
js_stream, | ||
ClientConnection::new(tls_config, hostname_dns)?, | ||
NonZeroUsize::new(65536), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, where does this come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It comes from here:
Lines 63 to 64 in c217928
pub(crate) const TLS_BUFFER_SIZE: Option<NonZeroUsize> = | |
NonZeroUsize::new(65536); |
Socket.prototype._unrefTimer = function () { | ||
// deno-lint-ignore no-this-alias | ||
for (let s = this; s !== null; s = s._parent) { | ||
for (let s = this; s != null; s = s._parent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these unrelated changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, _parent
can be undefined (not just null) when s is JSStreamSocket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, are there any other node compat tests we could enable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No new tests pass immediately with this PR
5fdaca8
to
321a52f
Compare
Thanks, thanks a lot for this 🙏🏼 |
Fixes #20594
This implements
JSStreamSocket
which drives the TLS underlying stream inrustls_tokio_stream
using 2 sets of channels. One for piping the encrypted protocol transport and the other for plaintext application data.This fixes connecting to
npm:mssql
: