uv_link_t: initial #32
Conversation
|
As far as I can see, this would tremendously help native addons writers also, since StreamBase is indeed very bulky. Decreasing boilerplate one has to implement to hook in C programs into js-streams is definitely worth an addition like this. Awesome work, btw. |
|
Indeed, this looks great! The more we can leverage libuv to do the heavy lifting generically, the better. 😸 |
|
|
||
| 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: |
|
oooo... yes please. this definitely looks promising. |
| ## 1. Proposal | ||
|
|
||
| I propose to replace `StreamBase` with [`uv_link_t`][0], and port existing | ||
| classes that inherit from `StreamBase`. |
There was a problem hiding this comment.
We had a conflict in StreamBase that lead to the method GetAsyncWrap() and GetObject(). Will this play nicely with AsyncWrap, and allow the class to propagate like it does with JSStream?
There was a problem hiding this comment.
The conflict was due to the use of vtable and virtual methods in StreamBase. uv_link_t is 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_t is 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.
|
Added note about observability and |
|
|
||
| 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 |
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
Actually, I'm wondering about the idea of prototyping this in userland, since uv_link_t, uv_http_t, and uv_ssl_t are all fully separate from libuv itself currently. Can we make totally separate modules for those three parts and have them interact cleanly? That'd be a great proof of modularity. It could be very interesting to see lots of little uv extensions like this exposed as userland modules, a uv_websocket_t link comes to mind. 😸
There was a problem hiding this comment.
This sounds lovely, @Qard . Though, we will still need to have access to the uv_stream_t which is currently hidden in core.
There was a problem hiding this comment.
Maybe we could add an internal/undocumented function somewhere to return a v8::External containing the uv_stream_t to experiment with the idea?
There was a problem hiding this comment.
@Qard do you think this should be a part of this proposal?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Overall I dig it. Would like @bnoordhuis' opinion. |
|
@trevnorris thank you! |
| 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 | ||
| dependencies. | ||
| envision how it will be used in Node.js addons. Even core modules will could |
There was a problem hiding this comment.
"will could" -> "could". Other than that minor thing, reads great. Thanks!
|
Thank you for review @trevnorris ! |
|
|
||
| `uv_link_t` aims to solve complexity problem that quickly escalates once | ||
| using multiple layers of protocols in [libuv][1] by providing a way to | ||
| implement protocols separately and chain them together in easy and |
|
The document should provide some detail on how uv_link_t is supposed to work. It currently just, ah, links to https://github.com/indutny/uv_link_t without further explanation. |
|
@bnoordhuis I can copy-paste "How?" from https://github.com/indutny/uv_link_t#how , will it suffice? |
|
Yes, but it should probably also say a few words about how it's going to be hooked up in node.js. |
|
@bnoordhuis sorry for delay, pushed update. |
|
Should I land this? Its been awhile since the last review, and I believe that there is a general agreement about this feature. |
Since it says status is DRAFT, and the README says all EPs regardless of status should be landed...I'd say yes. Landing it as DRAFT status especially should be uncontroversial. |
|
Alright, landing! |
|
Landed in 2c45bf0, thank you! |
cc @nodejs/ctc @trevnorris @nodejs/collaborators @saghul