-
Notifications
You must be signed in to change notification settings - Fork 30k
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
TLSCallbacks => TLSWrap, better TLS inception #840
Conversation
@bnoordhuis : would be cool to land ASAP, as it'll be impossible to rebase in case of major conflicts :) |
The interesting question is what semver tag should it have. Technically, there are some differences regarding the What are your thoughts, semver-patch/semver-minor/semver-major? |
One more thing! There are some C++ lint issues, but I think they are mostly a bugs of cpplint itself. |
I was using |
@wraithan I'm going to create a legacy property for this purpose right now. Thanks! |
@wraithan - done |
} | ||
socket.once('connect', onHandle); | ||
} | ||
socket = new TLSSocket(options.socket, { |
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.
there a reason this isn't just var socket =
instead of forward declaring it 2 lines above? Same for var result
which could be collapsed
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.
fixed!
@@ -437,49 +478,56 @@ TLSSocket.prototype._finishInit = function() { | |||
}; | |||
|
|||
TLSSocket.prototype._start = function() { |
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 it possible that the once
below could set multiple listeners for connect
, causing _start
to be called >1?
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.
Start is called only once, otherwise assertion will happen in C++.
@trevnorris mind helping with benchmark it against current v1.x? |
// Update handle if it was wrapped | ||
handle = self._handle; | ||
// TODO(indutny): assert that the handle is actually an ancestor of old one | ||
// assert(handle === self._handle, 'handle != self._handle'); |
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.
Does the following diagram encapsulate the behavior after wrapping?
A B C
Readable -> net.Socket -> TLSWrap -> TCPWrap
TCPWrap -> TLSWrap -> net.Socket -> Writable
D E F
A: cleartext bytes
B: cleartext bytes
C: encrypted bytes
D: encrypted bytes
E: cleartext bytes
F: cleartext bytes
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.
I think the Readble
/Writable
should be swapped with this direction of arrows, but generally this is correct.
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.
I'm using Readable
as "userland readable stream" – so bytes written to the socket will always hit TLSWrap on egress.
any numbers on performance for this? |
// Determine storage size first | ||
size_t storage_size = 0; | ||
for (size_t i = 0; i < count; i++) { | ||
Handle<Value> chunk = chunks->Get(i * 2); |
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.
Why use chunks->Length() >> 1
above but i * 2
here (instead of i << 1
)?
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.
This is a question for a separate issue, I was only migrating old code base here.
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.
Cool, just wanted to make sure I wasn't missing something.
@rvagg: @trevnorris is much better at this than I am, hopefully he will help us :) |
I read through it – it looks good to me, though I'd like to get a more experienced C++ reviewer's eyes on it. I confess I'm having a bit of a hard time conceptualizing the entire route from "write to TLSSocket in JS" to "write bytes to the network", and vice versa. If this works like I think it does, would it be possible to, for example, make the zlib transform streams nested in a similar fashion (by doing the compressing/decompressing work in the uv threadpool)? Or for a perhaps sillier point, to pass a duplex stream representing a FIFO into TLSSocket from JS? |
Yes, though, you won't be able to do zlib compress in the same uv_work_t call. It will be two separate thread pool requests. |
@chrisdickinson thanks for taking a look! |
@rvagg: benchmarks appears to be unaffected, which is great!
|
Another idea. What about exposing this API and adding a "standard" way to wrap stream? |
abf42e9
to
8eaf985
Compare
res[name] = function methodProxy() { | ||
return handle[name].apply(handle, arguments); | ||
}; | ||
}); |
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.
Not in love with this solution, but too lazy right now to suggest a better one.
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.
Can't say that I like it either. But the thing is that somehow HandleWrap
, StreamWrap
methods should be called. This definitely better be abstracted in JS.
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.
Will this not leak arguments, making optimization impossible?
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.
This usually applies to close
and connect
methods. Nothing performance critical.
d064fb2
to
07025b7
Compare
@trevnorris thank you! @bnoordhuis going to push it in a couple of days. I have already rebased it twice with merge conflicts :( |
Guess it is time to land it :) |
StreamBase is an improved way to write C++ streams. The class itself is for separting `StreamWrap` (with the methods like `.writeAsciiString`, `.writeBuffer`, `.writev`, etc) from the `HandleWrap` class, making possible to write abstract C++ streams that are not bound to any uv socket. The following methods are important part of the abstraction (which mimics libuv's stream API): * Events: * `OnAlloc(size_t size, uv_buf_t*)` * `OnRead(ssize_t nread, const uv_buf_t*, uv_handle_type pending)` * `OnAfterWrite(WriteWrap*)` * Wrappers: * `DoShutdown(ShutdownWrap*)` * `DoTryWrite(uv_buf_t** bufs, size_t* count)` * `DoWrite(WriteWrap*, uv_buf_t*, size_t count, uv_stream_t* handle)` * `Error()` * `ClearError()` The implementation should provide all of these methods, thus providing the access to the underlying resource (be it uv handle, TLS socket, or anything else). A C++ stream may consume the input of another stream by replacing the event callbacks and proxying the writes. This kind of API is actually used now for the TLSWrap implementation, making it possible to wrap TLS stream into another TLS stream. Thus legacy API calls are no longer required in `_tls_wrap.js`. PR-URL: #840 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Landed in b968623, thank you everyone! @bnoordhuis if you have any comments - feel free to open issue for them, I'll fix everything or we'll figure out how to do it best. |
Notable changes: * tls: A typo introduced in the TLSWrap changes in #840 only encountered as a bug on Windows was not caught by the io.js CI system due to problems with the Windows build script and the Windows CI slave configuration, see Fixed in #994 & #1004 (Fedor Indutny) * npm: Upgrade npm to 2.6.1. See https://github.com/npm/npm/blob/master/CHANGELOG.md#v260-2015-02-12 for details. * Add new collaborators: - Robert Kowalski (@robertkowalski) - Christian Vaagland Tellnes (@tellnes) - Brian White (@mscdex)
Ohai @iojs/crypto @iojs/streams .
This is very important thing for both of you! Please take a look at it, even if you don't think that you should actually review it.
Since introduction of TLSWrap concept and
_tls_wrap.js
/_tls_legacy.js
files, there was a big concept gap that needed to be filled. The TLSSocket itself is parasiting (consuming) the original TCP socket, making the reads from the raw TCP socket go directly into OpenSSL, thus improving the performance significantly.However, there was a major drawbacks:
This PR should solve first 4 issues, and a follow-up will solve the last one. The main idea of a fix is to separate concept of
StreamWrap
C++ class from theHandleWrap
class. I.e. make C++ streams independent of internal libuv socket/pipe. Next step that it does is a TLSWrap C++ stream that consumes the input of the input stream and proxies writes to itself to the underlying TCP stream (performing encryption and TLS protocol on this way).4th step will be fixed by introducing JSStreamWrap C++ class, which will emulate C++ input events from JS-land.
Please take a look and review!
Thank you.