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

Fix issues with node 0.12 #73

Merged
merged 3 commits into from Mar 4, 2015
Merged

Fix issues with node 0.12 #73

merged 3 commits into from Mar 4, 2015

Conversation

@kanongil
Copy link
Member

kanongil commented Feb 23, 2015

The main issue is that calls to abort() can now cause the stream to stop without any notification at all. As a consequence, I have implemented a workaround that intercepts all calls to abort().

The SSL test fix might be specific to my homebrew 0.12 install, though it should not cause any issues.

kanongil added 2 commits Feb 23, 2015
This fixes 2 failing test cases, and adds a new related non-failing case
@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 23, 2015

!!! Thanks so much @kanongil

@geek geek added the bug label Feb 23, 2015

var _abort = req.abort;
var aborted = false;
req.abort = function () {

This comment has been minimized.

Copy link
@geek

geek Feb 23, 2015

Member

not a fan of changing the node req object. Can you find another way to solve this?

This comment has been minimized.

Copy link
@kanongil

kanongil Feb 23, 2015

Author Member

Unfortunately not, as far as I can tell. This can only be resolved in node itself. You are welcome to submit a bug report.

As it stands, if abort() is called before the socket has been assigned, node >= 0.12 will silently drop the request without any further emits on the request object. This is all handled in https://github.com/joyent/node/blob/master/lib/_http_client.js.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Feb 24, 2015

Member

Who else can call abort() other than line 87? Because is no one, then why override the node internals?

This comment has been minimized.

Copy link
@kanongil

kanongil Feb 24, 2015

Author Member

abort() is part of the exposed api in the req object that is returned. It should definitely be supported. Without this patch an early abort() call can cause the request callback to never be called.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Feb 24, 2015

Member

But what is the use case? It's one thing to protect the wreck internals but I don't think wreck users should be calling abort.

This comment has been minimized.

Copy link
@kanongil

kanongil Feb 24, 2015

Author Member

That is more of a question for node. It is part of the public api, and I guess they have a reason for it to be there. See http://nodejs.org/api/http.html#http_request_abort. It is inherently also part of the wreck api, with test cases to support it. Also, there is no way to remove this.

My primary concern is that without this (or similar) patch, users can experience obscure timing related bugs when using abort. It is quite bad form to have a callback that is not always called.

I definitely agree that it sucks to do this, and I ultimately think that this should be solved in node. Until then I think we have 3 options:

  1. Use this patch
  2. Drop the abort tests & document divergent abort behavior
  3. Drop 0.12 support

Finally, there is a small possibility that I'm wrong about the missing notification and it can be solved without a hack. You are welcome to look for it.

.travis.yml Outdated
@@ -1,5 +1,6 @@
language: node_js

node_js:
- 0.10
- "0.10"
- "0.12"

This comment has been minimized.

Copy link
@hueniverse

hueniverse Feb 24, 2015

Member

might as well add iojs

@kanongil kanongil force-pushed the kanongil:0.12-compat branch from a405c10 to 865900c Feb 24, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Feb 24, 2015

@chrisdickinson @cjihrig Can you chime in?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Feb 24, 2015

Based on a little poking around, this seems to be a bug to me (although Chris may disagree). There is req.aborted, but you would have to watch it. It would be nice if node emitted an abort event.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Feb 24, 2015

@cjihrig An abort event would only be for convenience. It should always emit an error, like in 0.10.x.

Given that this is a node 0.12+ problem, we could actually detect the abort using Object.observe(). I am not sure if this would make for a better solution, though.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Feb 24, 2015

And what a convenience that would be right now :-) I don't think explicitly aborting a request should result in an error, but that is beside the point.

I wrote the Object.observe() detection code. It's simple but not backwards compatible and seems hacky.

@chrisdickinson

This comment has been minimized.

Copy link

chrisdickinson commented Feb 25, 2015

@cjihrig +1 on an "abort" event, though that would still leave us with a few patch versions of Node that wreck would not support due to this bug. Is that acceptable?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Feb 25, 2015

@geek is the lead maintainer, so it's his call. Could always use a hackier approach until official support lands. FWIW, nodejs/node-v0.x-archive#9278 and nodejs/node#945

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Feb 25, 2015

Is this a bug in node 0.12? If it is, can we just fix it there and then note the version requirements?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Feb 26, 2015

This has landed in io.js. TJ is OK with the change to Node, but said that it can't go into 0.12. His recommendation was to monkey patch abort() in wreck to emit the abort event.

@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 26, 2015

@cjihrig what was the reason for not allowing it in 0.12?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Feb 26, 2015

It's technically an API change.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Mar 3, 2015

Seems like we got our answer. What's the proposed solution to make this work across 0.10, 0.12, and io?

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Mar 3, 2015

As far as I can tell we are down to monkey patching, as I did and TJ suggests, and using Object.observe() which will be difficult to provide 100% test coverage on.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Mar 3, 2015

Given that TJ's suggestion was to monkey patch abort(), I guess it depends if we want to maintain 0.10 behavior. If so, this patch is appropriate. There won't be an abort event in 0.12, and the event in io.js can simply be ignored.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Mar 3, 2015

I'll let @geek decide how to proceed.

@geek geek added this to the 5.3.0 milestone Mar 4, 2015
@geek geek self-assigned this Mar 4, 2015
geek added a commit that referenced this pull request Mar 4, 2015
Fix issues with node 0.12
@geek geek merged commit 215a89e into hapijs:master Mar 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 4, 2015

@kanongil this will be published with v5.3.0. Thanks for fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.