fix: max httpApi timeout #10119
fix: max httpApi timeout #10119
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10119 +/- ##
==========================================
+ Coverage 85.27% 85.38% +0.11%
==========================================
Files 335 339 +4
Lines 13687 13819 +132
==========================================
+ Hits 11671 11800 +129
- Misses 2016 2019 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@CaioFauza thanks for an initiative, still please see my comment
| @@ -642,7 +642,7 @@ Object.defineProperties( | |||
| // It's a margin needed for some side processing time on AWS side. | |||
| // Otherwise there's a risk of observing 503 status for successfully resolved invocation | |||
| // (which just fit function timeout setting) | |||
| routeTargetData.timeout = Math.min(functionTimeout + 0.5, 29); | |||
| routeTargetData.timeout = Math.min(functionTimeout + 0.5, 29.5); | |||
There was a problem hiding this comment.
This will allow 29.5 as endpoint timeout, while maximum supported on HTTP API side is 29
Also please see my comment at: #9677 (comment)
There was a problem hiding this comment.
Here simply let's also update it to 30
| @@ -642,7 +642,7 @@ Object.defineProperties( | |||
| // It's a margin needed for some side processing time on AWS side. | |||
| // Otherwise there's a risk of observing 503 status for successfully resolved invocation | |||
| // (which just fit function timeout setting) | |||
| routeTargetData.timeout = Math.min(functionTimeout + 0.5, 29); | |||
| routeTargetData.timeout = Math.min(functionTimeout + 0.5, 29.5); | |||
There was a problem hiding this comment.
Here simply let's also update it to 30
Done! I just added one more unit test to grant coverage to the new 30s case as well. |
Closes: #9677
The issue pointed out that the time margin was not being added to the maximum timeout value. The Math.min method was considering 29 as the maximum value, therefore, when passing the limit value, the additional 0.5 seconds was not considered.
I also added a new unit test to consider the case.