Skip to content
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

Remove custom output parsing and buffer payload chunks via write() #79

Merged
merged 8 commits into from Sep 23, 2016

Conversation

@mtharrison
Copy link
Member

mtharrison commented Sep 9, 2016

For review:

This is an attempt to cleanup the way shot dips into the Node HTTP internals and to avoid the custom HTTP parsing we have in place.

I've tested with current hapi master on node 4.x.x and node 6.x.x and all tests pass.

It should make #78 easier to implement too.

@mtharrison mtharrison added the feature label Sep 9, 2016
@@ -14,6 +14,8 @@ exports = module.exports = internals.Response = function (req, onEnd) {

Http.ServerResponse.call(this, { method: req.method, httpVersionMajor: 1, httpVersionMinor: 1 });

this.shot = { trailers: {}, payloadChunks: [] };

This comment has been minimized.

Copy link
@mtharrison

mtharrison Sep 9, 2016

Author Member

If someone feels squeamish about extending a builtin here, we could use a WeakMap instead.

This comment has been minimized.

Copy link
@devinivy

devinivy Sep 9, 2016

Member

I don't mind this. Maybe privatize it with an _? Also, any reason headers don't also live in here?

This comment has been minimized.

Copy link
@mtharrison

mtharrison Sep 9, 2016

Author Member

👍

This comment has been minimized.

Copy link
@mtharrison

mtharrison Sep 9, 2016

Author Member

The response._headers is keeping the contract with the Node object, and I didn't see the need to duplicate them.

This comment has been minimized.

Copy link
@devinivy

devinivy Sep 9, 2016

Member

Ohhp, I see now!

@@ -53,6 +55,7 @@ internals.Response.prototype.writeHead = function () {

internals.Response.prototype.write = function (data, encoding) {

this.shot.payloadChunks.push(new Buffer(data, encoding));

This comment has been minimized.

Copy link
@devinivy

devinivy Sep 9, 2016

Member

It does bother me a little bit that the ServerResponse will also keep its own buffer (as long as it thinks it doesn't have a socket), so that data will be duplicated in memory.

This comment has been minimized.

Copy link
@mtharrison

mtharrison Sep 9, 2016

Author Member

What do you think of 7e66b76 ?

This comment has been minimized.

Copy link
@devinivy

devinivy Sep 9, 2016

Member

It excites me.

@@ -84,91 +95,12 @@ internals.payload = function (response) {
trailers: {}
};

// Read payload

This comment has been minimized.

Copy link
@devinivy

devinivy Sep 9, 2016

Member

Dropping all this code is brilliant 👍

mtharrison added 7 commits Sep 9, 2016
@mtharrison

This comment has been minimized.

Copy link
Member Author

mtharrison commented Sep 19, 2016

@hueniverse do you have any objections to this patch aside from the concern mentioned here #78 (comment) about messing with how Shot works.

This PR removes some of the games mentioned there by removing all the protocol parsing stuff and buffering the payload directly in shot rather than reaching into the output stream. It feels like an improvement and relatively low-risk to me.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Sep 19, 2016

Did you test it with hapi and h2o2?

@mtharrison

This comment has been minimized.

Copy link
Member Author

mtharrison commented Sep 19, 2016

Yes, tested with both.

@hueniverse hueniverse self-assigned this Sep 23, 2016
@hueniverse hueniverse added this to the 3.3.2 milestone Sep 23, 2016
@hueniverse hueniverse merged commit ccc2426 into hapijs:master Sep 23, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mtharrison mtharrison deleted the mtharrison:no-stream-internals branch Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.