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

response.streamify assumes stream has attribute _readableState #2368

Closed
Thomathan opened this issue Jan 23, 2015 · 10 comments
Closed

response.streamify assumes stream has attribute _readableState #2368

Thomathan opened this issue Jan 23, 2015 · 10 comments
Assignees
Labels
Milestone

Comments

@Thomathan
Copy link

@Thomathan Thomathan commented Jan 23, 2015

If this attribute doesn't exist, it throws TypeError: Cannot read property 'objectMode' of undefined

It should check if _readableState exists before it checks for objectMode.

Below is the relevant function and the diff for the fix needed.

Hapi: 8.1.0
Node: 0.10.35
npm: 2.1.18
File: hapi/lib/response.js

internals.Response.prototype._streamify = function (source, next) {

    if (source instanceof Stream) {
        var stream = (source.socket || source);
        if (stream._readableState.objectMode) {
            return next(Boom.badImplementation('Cannot reply with stream in object mode'));
        }

        this._payload = source;
        return next();
    }

    // other code
}

Diff

    451c451
    <         if (stream._readableState.objectMode) {
    ---
    >         if (stream._readableState && stream._readableState.objectMode) {
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 23, 2015

What are you passing that's missing the attribute?

@Thomathan
Copy link
Author

@Thomathan Thomathan commented Jan 23, 2015

I'm replying with a request stream from request package (https://github.com/request/request).

      reply(request.get(url))

I have tried using the request to pipe into reply but found that corrupted the file i was trying to reply with.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 23, 2015

@nlf wasn't this covered in your fix in 8.1?

@nlf
Copy link
Member

@nlf nlf commented Jan 23, 2015

Should've been, I think. Will look again.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 23, 2015

I might have broke it in my refactor but please check.

@kanongil
Copy link
Contributor

@kanongil kanongil commented Jan 30, 2015

Streams are not required to have a _readableState state property. They will get this if the implementation inherits from stream.Readable, but otherwise there is no such guarantee.

If hapi wants to support all streams, you can't depend on this property. If you only want to support streams2+, you need to do additional checks like here, and return an appropriate error when a classic stream is used.

@Thomathan You should be able to solve your issue using readable.wrap().

kanongil added a commit to kanongil/hapi that referenced this issue Jan 30, 2015
This replaces "TypeError: Cannot read property 'objectMode' of undefined" error response

Fixes hapijs#2368 & hapijs#2301
@hueniverse hueniverse added the bug label Feb 9, 2015
@hueniverse hueniverse added this to the 8.2.0 milestone Feb 9, 2015
@hueniverse hueniverse removed this from the 8.2.0 milestone Feb 9, 2015
@kanongil
Copy link
Contributor

@kanongil kanongil commented Feb 9, 2015

@Thomathan Did you solve your issue by using wrap, or you waiting for Hapi to support the old streams interface again?

@Thomathan
Copy link
Author

@Thomathan Thomathan commented Feb 10, 2015

@kanongil I've been busy working on other things and have not tried wrap yet. However, I will be using that to fix my problem, at least until request updates to streams2+

kanongil added a commit to kanongil/hapi that referenced this issue Feb 20, 2015
This replaces "TypeError: Cannot read property 'objectMode' of undefined" error response

Fixes hapijs#2368 & hapijs#2301
kanongil added a commit to kanongil/hapi that referenced this issue Feb 20, 2015
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
@arb
Copy link
Contributor

@arb arb commented Jul 4, 2015

While this issue has been long closed, I just want to follow up that yes, using Readible.wrap does allow you to stream results from the node-request module.

@kanongil
Copy link
Contributor

@kanongil kanongil commented Jul 4, 2015

Thanks for the feedback @arb. In my investigations I did find that it it seems to work but as far as I remember I concluded that it is safer to stream the ClientRequest object returned from the response event. This also allows you to handle http status codes and other header information.

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

Successfully merging a pull request may close this issue.

5 participants