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

timeout for payload not working. #19

Closed
lintuxt opened this issue Oct 5, 2015 · 6 comments · Fixed by #23
Assignees
Labels
bug
Milestone

Comments

@lintuxt
Copy link

@lintuxt lintuxt commented Oct 5, 2015

Hello, I have this configuration object for Hapi.js route

    method: 'POST',
    path: '/upload',
    handler: POSTHandlers.upload,
    config: {
        payload: {
            output: 'stream',
            parse: true,
            allow: ["multipart/form-data"],
            maxBytes: 209715200,
            timeout: 4000
        }
    }

I believe that in the Wreck call is the issue:
Wreck.read(part, {timeout: self.settings.timeout ... is needed.
https://github.com/hapijs/subtext/blob/master/lib/index.js#L370-L427

But also, once I get to fire the timeout error, the call to self.next(Boom ... ) is not being correctly propagated.

Am I doing something wrong?

Thank you.

@johnbrett johnbrett self-assigned this Oct 6, 2015
@johnbrett johnbrett added the bug label Oct 7, 2015
@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Oct 7, 2015

Hi @lintuxt, interesting spot here. That part of the code isn't touched for file upload, if you see https://github.com/hapijs/subtext/blob/master/lib/index.js#L343-L369 this is actually the part for dealing with files, so the timeout needs to be applied to either the writeFile or Pez. Will look into this further.

@lintuxt

This comment has been minimized.

Copy link
Author

@lintuxt lintuxt commented Oct 7, 2015

Actually it depends on what kind of output in the configuration object you are using. In my case I'm using stream because I want to handle the writing to disk myself. Also be aware that even though you can handle this situation. There is another problem. hapijs/hapi#2825 . The next() call won't work until request has finished. Cheerz!

johnbrett added a commit that referenced this issue Oct 8, 2015
Failing test for timeout on stream multipart payload for issue 19:
#19
johnbrett added a commit that referenced this issue Oct 8, 2015
Added fix for issue: #19

Also updated test to match hapi style guide
@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Oct 8, 2015

@lintuxt I've added the fix suggested, it just failed likely to a timing issue. I will get to that, and I also want to test how this change will affect hapi to before merging in. Will hopefully get to it before the weekend.

@lintuxt

This comment has been minimized.

Copy link
Author

@lintuxt lintuxt commented Oct 9, 2015

Thank you so much John, please let me know if you need any help!

@johnbrett johnbrett added this to the 2.0.3 milestone Oct 30, 2015
@hueniverse hueniverse removed this from the 2.0.3 milestone Nov 3, 2015
@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Nov 6, 2015

Hoping to get to this over weekend, sorry for delay on this!

johnbrett added a commit that referenced this issue Nov 14, 2015
Adding failing tests for #19
@johnbrett

This comment has been minimized.

Copy link
Contributor

@johnbrett johnbrett commented Nov 14, 2015

Apologies for delay on this @lintuxt, fixed via #23

@johnbrett johnbrett added this to the 3.0.1 milestone Nov 14, 2015
johnbrett added a commit to hapijs/hapi that referenced this issue Nov 14, 2015
This contains a fix in subtext where timeouts weren't respected on multipart payloads, fixing hapijs/subtext#19. 

Related: #2825 and outmoded/discuss#171
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.