-
Notifications
You must be signed in to change notification settings - Fork 9
Making http "hookable" #6
Comments
The alternative on the first two bullet points above is to make the custom header handling part of the alternative parser implementation... e.g.
|
It would be nice to have the parser be per-instance, not global, because being able to globally override behavior, while nice for hotfixes to issues, is frustrating when trying to make defensive, reliable modules that use an API that can be plugged. |
The header parsing of the response from the server could be one thing which can be made hookable. |
Why not just expose the existing API that the current http parser is/was using? Namely, |
@mscdex ... yep, that's definitely a solid option (in fact, it should be the default option unless a better choice comes along later). Essentially, we'd have a new API for plugging in a replacement parser at the JS level that implemented those same three methods. |
While I agree with this theoretically, the overhead could prove to be substantial on the existing implementation. Unless it can be done w/o near-zero overhead then I doubt much of the general audience will go for it.
What if the header field parser is insecure? IOW, sort of the point of this is to (pretty much) remove responsibility from node in terms of securely parsing. Also it would remove the opportunity to do any performance enhancements in the future. I've been playing with a couple ideas that could offer substantial gains, but after a change like this those improvements are unlikely to ever land. I saw the easiest route to bludgeon the issue in the face and remove everything between the TCP data event and the http server http.setNewServer(callback);
http.setOnRequest(server, socket, req); Those names are horrible, but we can look past that. Where Just an idea, and still very rough, but figured that type of approach would ease the necessary aspect of separating the HTTPParser from the other list of changes. |
Is there really a strong case of swapping the core http implementation? Why not just let people ignore it. If you are using a module that directly accesses http, then let it. It will work just fine and the API contract will not be a concern. |
@hueniverse ... essentially make it easier for modules to directly access the underlying stream/socket then? Correct? |
Asked myself the same question and an applicable reason my be so any other modules that use the native http module will automatically inherit the changes. e.g. require('my-custom-http');
var express = require('express'); As a way to enforce certain header parsing policies. |
Isn't that too much spooky action at a distance? I'd extend express so it can take a custom http implementation. |
I can appreciate not allowing spooky action, but wouldn't any type of http hooks result in the same? |
Not when you have to explicitly configure express. The spooky action I'm thinking of is a module three levels deep quietly making process-wide changes. |
Then the premise of
is no longer valid. |
My requirement is to be able to write my own http server without having to write all the TCP and TLS pieces. |
@hueniverse would you mind giving a few brief points on how you think that would look? I assume you mean not needing to handle the |
I'll need to refresh my knowledge on the current code. Hope to do that next few days. |
@nodejs/http ... Pulled over to a new issue from #2 (comment)
One of the possible approaches we can take with the http module is allowing third party modules to systematically replace core functions of the http implementation with their own bits. To do this we would need to:
To keep things as simple as possible, I'd recommend that we identify the replaceable pieces as coarsely as possible. This is just a strawman at this point.... feel free to knock it down.
const http = require('http'); http.useParser(myParser);
)const http = require('http'); http.onHeader('x-foo', myFooParser);
)Not quite sure yet what all would need to happen on the client side... still working through that. Thoughts?
The text was updated successfully, but these errors were encountered: