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

Return explicit error when trying to stream a non-Readable stream #2382

Merged
merged 1 commit into from Mar 5, 2015

Conversation

@kanongil
Copy link
Contributor

@kanongil kanongil commented Jan 30, 2015

Fix #2368 & #2301 by returning an explicit error for nonReadable streams instead of throwing.

This also fixes buggy behavior introduced in #2302, where a Writable only stream can be accepted as long as it contains a socket._readableState object.

@@ -447,8 +447,11 @@ internals.Response.prototype._marshal = function (next) {
internals.Response.prototype._streamify = function (source, next) {

if (source instanceof Stream) {
var stream = (source.socket || source);
if (stream._readableState.objectMode) {
if (typeof source._read != 'function' || typeof source._readableState != 'object') {
Copy link
Contributor

@hueniverse hueniverse Feb 9, 2015

Can't we just check for instance of Readable?

Copy link
Contributor Author

@kanongil kanongil Feb 9, 2015

Unfortunately not. We need to rely on duck typing to support the common case of Readable streams created through the readable-stream module, which are not an instance of require('stream').Readable.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 9, 2015

Will this work with the request module?

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 9, 2015

This will error if request module is used, as it is not streams2+. If we want to support that directly, we need to drop the streams2 requirement, maybe by adding an autoWrap option.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 9, 2015

But request streams work today, right? I would rather leave things alone and just be more flexible in how we detect object mode than block on all older streams. I don't want this to be a breaking change.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 9, 2015

No, request streams does not work as argument to reply. It results in:

Debug: internal, implementation, error 
    TypeError: Cannot read property 'objectMode' of undefined
    at internals.Response._streamify…

You have to wrap them, though I guess more commonly people use the buffered response from the callback.

Update: This is not a breaking change (unless you count the 8.1.0+ acceptance of Writable only streams returned from Http.get()). I am just trying to return a more appropriate error message that indicates the reason for failing. Eg. it must either have been a streams1 Stream, a streams2+ Writable stream, or a completely unknown stream type.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 10, 2015

Well, request streams used to work until we added the object mode test. So this entire thing has always been about how to test for object mode with minimal impact. I rather remove the object mode assertion than keep all this shit around.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 10, 2015

Sure, I don't mind – I just thought that you had a specific reason for the "streams2" compatible requirement mentioned in the API docs. This requirement breaks request stream support.

Do you still want to throw explicit errors for objectMode streams? If this check is disabled, objectMode streams will currently hang the client, and report an internal error like this:

Debug: internal, implementation, error 
    TypeError: first argument must be a string or Buffer
    at ServerResponse.OutgoingMessage.write (http.js:852:11)
    at write (_stream_readable.js:602:24)
    at flow (_stream_readable.js:611:7)
    at _stream_readable.js:579:7
    at process._tickDomainCallback (node.js:486:13)

What about Writable only streams2 streams like those returned from Http.get? Currently, they just hang the client forever. I would prefer an explicit error response.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 16, 2015

I'd like a solution that keeps request working while giving the user some error about other invalid streams.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 16, 2015

By "keeps it working", I assume you mean "makes it work again"?

An option for this, is to detect non-streams2 streams, and convert them to streams2 using wrap(). Maybe adding the autoWrap option that I suggested at some point?

The advantage of this approach is that all streams are streams2 internally, while allowing legacy streams to be used. On the flip side, it will add some bloat only to save people from wrapping the legacy streams themselves, calling reply(new Readable().wrap(stream)).

Let me know which approach you think is more suitable, and I will try to update the patch.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 16, 2015

I don't want to do anything to the stream. Basically, try to figure out if it is a readable stream (1 or 2) and if it is not stream2 in object mode. Anything else just gets passed along.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 16, 2015

I think I understand what you want. This requires a review of all the internal stream processing to ensure it doesn't rely on the streams2 interface. At least the "close" handling has this dependency.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 16, 2015

It doesn't. It just calls pipe().

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 17, 2015

Ok, I spent quite a bit of time adding test cases for classic streams, which has led me to the conclusion that Hapi will have a hard time supporting classic streams on the current architecture, given that async processing takes place between reply(stream) is called, and the stream is piped.

I think the most sensible approach is to use the wrap function immediately to convert it to the new API and catch any immediate data emits. This also has the side effect of disabling the statusCode and header rewriting request automatically applies to the piped response.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 17, 2015

Passing along the status code from request was an actual feature added to hapi a long time ago. What's the status of this PR at this point?

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 17, 2015

Is this feature actually tested in a meaningful manner? Are you referring to the passThrough response option?

The PR is mostly complete. One iffy part is this passThrough option handling. I included some code that will copy the statusCode and headers from legacy streams but it depends on these being available when the stream is passed to the reply.

The other iffy part is that it breaks the api when rewriting the source. Maybe we should just create a new internal variable to hold this stream? This would also make the passThrough hack redundant. Actually, let me have a look at doing that.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 17, 2015

Ok, this turned out better than I thought it would. I managed to reuse the existing _payload property to store the internal Readable.

The only outstanding issue is the missing request pipe header forwarding.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 17, 2015

There is already logic to forward the status and headers. It just looks for a headers property on the stream.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 17, 2015

I know. I think I prefer that over the non-configurable request pipe forwarding.

The only issue with the Hapi code (as it is right now), is that it requires the values to be resolved before the processing is started. Eg. for reply(Request.get('http://google.com')) there is no way to tell beforehand whether they will used. You will need to do something like this to ensure that they are ready:

var ready = function(res) {
  this.removeEventListener('response', ready);
  this.removeEventListener('error', ready);
  reply(res);
};
Request.get('http://google.com').on('response', ready).on('error', ready);

Incidentally, this code also works on the current Hapi release since it forwards the response stream.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 17, 2015

We are not trying to make it easier to use request, just to make sure we support the stream returned by request when used properly via the event or callback.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 17, 2015

I found another pitfall. If you use the base request object, Hapi will include the any request headers in the response, instead of the response headers. Hapi is probably a bit too eager to forward any header-like object it finds.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 17, 2015

Hmm, I believe this header forwarding behavior can be seen as a security issue. For instance a handler like:

reply(Request.get('http://local.service/protected-data', { headers: { 'x-api-key': 'sekrit' } });

will return the x-api-key header to the client.

While this is the documented behavior (request assigns the request header to a .headers property), it seems like a highly dangerous default.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 17, 2015

I don't think it is. It is documented and has been this way for a long time. There is a flag to turn it off. Also, the example you give it a bug, no? You should not reply with the return request stream.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 17, 2015

I think you are going down a rat hole with this. The issue we need to fix is pretty limited.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 19, 2015

Ok, it seems there is some confusion on how request can be used. It has 3 interfaces to the response data:

  1. Consume the stream returned from the call. This exposes a classic stream interface which emits data and can be piped.
  2. Consume the plain http response stream emitted with the response event.
  3. Use the Buffer/JSON data in the callback.

Hapi already supports 2 & 3. It seems that you are not aware of 1, which is the only case I'm trying to fix in this patch.

The first variant of this patch just ensured a proper error was returned for case 1. The new revision instead tries to support this and any other classic stream.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 19, 2015

If I just remove the assert for object mode and not call wrap on the first option, what will happen?

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 20, 2015

Then you will allow classic streams.

While this will work under some circumstances, it can lose some of the initial stream data under others. This is what most of my new code deals with.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 20, 2015

I don't want to deal with it. People should stop using the request module. It's crap until they clean it up and make it work correctly with modern node (as in last 2 years). I rather put a note in the docs that you need to call wrap() in that use case and just do a simple assertion.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 20, 2015

Great, I guess we are back to my initial patch? Good thing I kept it around then ;-)

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Feb 20, 2015

The patch is back to the initial conservative approach of erroring on non-compatible streams (including request type 1 streams).

I have incorporated some of the extra tests i developed for the classic streams support into this patch as well. Please note that the new transmit.js test exposes an internal hapi error (see https://travis-ci.org/hapijs/hapi/jobs/51567581). This is not a bug introduced with this patch, as the same test will error on the existing codebase.

This replaces "TypeError: Cannot read property 'objectMode' of undefined" error response

Fixes hapijs#2368 & hapijs#2301
@hueniverse hueniverse added this to the 8.3.0 milestone Mar 5, 2015
@hueniverse hueniverse self-assigned this Mar 5, 2015
hueniverse pushed a commit that referenced this issue Mar 5, 2015
Return explicit error when trying to stream a non-Readable stream
@hueniverse hueniverse merged commit 59922cb into hapijs:master Mar 5, 2015
1 check passed
hueniverse pushed a commit that referenced this issue Mar 5, 2015
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants