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

Set http timeout to 30 mins. #240

Merged
merged 1 commit into from
May 10, 2017
Merged

Set http timeout to 30 mins. #240

merged 1 commit into from
May 10, 2017

Conversation

rc1
Copy link
Contributor

@rc1 rc1 commented Apr 24, 2017

By default aws only allows the http request 2 minutes to upload. This PR increases the limit to 30 minutes. It will also report when a request is being retried and including the reason why.

I was trying to upload 8megs at home. It was taking 8 minutes to report a fail. This was because it uploads for 2 mins, waiting for 1 min and attempts twice more.

Thanks.

Copy link
Collaborator

@DeviaVir DeviaVir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 30 mins the max allowed, or an arbitrary number?
Would you mind adding tests for your changes?

Thanks for your contribution!

@rc1
Copy link
Contributor Author

rc1 commented Apr 25, 2017

It's just an arbitrary number. There is no limit from amazon.

I'm not sure what the additional tests would be? This PR is not expositing any additional functionality.

@DeviaVir
Copy link
Collaborator

@rc1 I was thinking about two tests, one to see if aws.config.httpOptions.timeout is being set (correctly), and one that would test the retry functionality you've added.

@rc1
Copy link
Contributor Author

rc1 commented Apr 25, 2017

To clarify, I've not added the retry functionality. It's doing that already. I'm am just logging the event to the console.

Being honest, I was just playing with a couple of free hours at home. I'd love to dig into the testing and figure it out... but I won't have time it the moment. Feel free to close.

@DeviaVir
Copy link
Collaborator

@rc1 apologies, it was not my intention to over-complicate things or to make this repo seem unreachable. I definitely want this change in as it makes node-lambda more usable for a broader public. Let me take a moment to see what kind of tests make sense and I'll be sure to add them on top of your changes.
Again, much appreciated that you spent some of your free time on this project.

@rc1
Copy link
Contributor Author

rc1 commented Apr 25, 2017

@DeviaVir no apologies needed. It is a bit lame on my part.

Ingoring this PR, it would be helpful to have either aws.config.httpOptions.timeout changed or changeable.

@DeviaVir DeviaVir merged commit c0369f9 into motdotla:master May 10, 2017
@rc1 rc1 deleted the fix-timeout branch May 15, 2017 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants