-
-
Notifications
You must be signed in to change notification settings - Fork 65
uv_link_t: initial #32
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| | Title | Port core to uv_link_t | | ||
| |--------|-----------------------------| | ||
| | Author | @indutny | | ||
| | Status | DRAFT | | ||
| | Date | 2016-06-03 | | ||
|
|
||
| ## 1. Proposal | ||
|
|
||
| I propose to replace `StreamBase` with [`uv_link_t`][0], and port existing | ||
| classes that inherit from `StreamBase`. | ||
|
|
||
| ## 2. Current state of C++ Streams in Node | ||
|
|
||
| The fast HTTP and TLS protocol implementation in Node core depends on so called | ||
| `StreamBase` C++ class. This class is an analog of JavaScript streams in C++. | ||
| The main ideas behind `StreamBase` is to: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: are to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack. |
||
|
|
||
| 1. Avoid unnecessary memory allocations by reading data directly into buffer | ||
| from which it will be synchronously parsed | ||
| 2. Avoid calling JavaScript unless absolutely necessary | ||
|
|
||
| However, this API has lots of dependencies on the core internals, and thus | ||
| can't be used outside of it. | ||
|
|
||
| ## 3. uv_link_t | ||
|
|
||
| From [`uv_link_t`][0] readme: | ||
|
|
||
| Chainable libuv streams. | ||
|
|
||
| It is quite easy to write a TCP server/client in [libuv][0]. Writing HTTP | ||
| server/client is a bit harder. Writing HTTP server/client on top of TLS | ||
| server could be unwieldy. | ||
|
|
||
| `uv_link_t` aims to solve complexity problem that quickly escalates once | ||
| using multiple layers of protocols in [libuv][0] by providing a way to | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack. |
||
| implement protocols separately and chain them together in easy and | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "in an easy and..."
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack. |
||
| high-performant way using very narrow interfaces. | ||
|
|
||
| Given that [`uv_link_t`][0] depends only on [libuv][1], it is very easy to | ||
| envision how it will be used in Node.js addons. Even core modules will no longer | ||
| be required to be distributed with core, since they could be built without core | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a hint of a concern that people will take this as meaning core modules will be distributed without core. Possibly change the wording to simply say that it'd be easier to write alternative implementations of core modules (e.g. alternative http implementations). Only suggest this to keep the proposal closer to core today (as an easier way for this EP to get accepted). note: i'm not suggesting that i wouldn't like to distribute the core modules separately in the future. :)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack, PTAL.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I'm wondering about the idea of prototyping this in userland, since
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds lovely, @Qard . Though, we will still need to have access to the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we could add an internal/undocumented function somewhere to return a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Qard do you think this should be a part of this proposal?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure yet. It's more just an idea in my head at this point. It might make acceptance of this EPS a little smoother though. We could just start with a single hidden function and iterate in userland for a bit. It'd provide the opportunity to develop a more practical understanding of the potential implementation than just bikeshedding a bunch in here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @indutny. This is an idea that has a lot of potential and lots of ways we can be extended. Think it will be good to be strict about not allowing these things to creep so the initial implementation can land. At that point I think it'll be much easier to begin prototyping new ways of doing things. |
||
| dependencies. | ||
|
|
||
| ## 4. Proof-of-Concept | ||
|
|
||
| Several modules were created as a Proof-of-Concept [`uv_link_t`][0] | ||
| implementations (which possibly could be modified to fit into core): | ||
|
|
||
| * [`uv_ssl_t`][2] | ||
| * [`uv_http_t`][3] | ||
|
|
||
| They can be combined together quite trivially as demonstrated in | ||
| [file-shooter][4]. | ||
|
|
||
| ## 5. Further Thoughts | ||
|
|
||
| Another abstraction level could be added for multiplexing [`uv_link_t`][0]s to | ||
| provide a common C interface for http2 and http1.1. This will help us bring | ||
| a http2 implementation into the core while reusing as much of the existing code | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as much as?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack, rephrased. |
||
| as possible. | ||
|
|
||
| [0]: https://github.com/indutny/uv_link_t | ||
| [1]: https://github.com/libuv/libuv | ||
| [2]: https://github.com/indutny/uv_ssl_t | ||
| [3]: https://github.com/indutny/uv_http_t | ||
| [4]: https://github.com/indutny/file-shooter | ||
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.
We had a conflict in
StreamBasethat lead to the methodGetAsyncWrap()andGetObject(). Will this play nicely withAsyncWrap, and allow the class to propagate like it does withJSStream?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.
The conflict was due to the use of
vtableandvirtualmethods inStreamBase.uv_link_tis a C project, there won't be any such conflict.In fact, I believe that exposing an External getter for the pointer to internal
uv_link_tis enough to get things running. This getter can be a per-class, or could be shared, but the inheritance won't be a roadblock for this.