Close HTTP response on unhandled request timeout#2007
Close HTTP response on unhandled request timeout#2007seratch merged 2 commits intoslackapi:mainfrom suhailgupta03:fix/timeout-response-handling
Conversation
This commit updates the defaultUnhandledRequestHandler to close the HTTP response when an incoming event is not acknowledged within the expected timeframe. This change ensures that resources are not left hanging and that the server correctly indicates to the client that the event was not processed as expected. By sending a 503 Service Unavailable status code and ending the response, we provide clearer semantics and prevent potential resource leaks on the server.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2007 +/- ##
==========================================
- Coverage 82.00% 81.97% -0.04%
==========================================
Files 18 18
Lines 1528 1531 +3
Branches 439 440 +1
==========================================
+ Hits 1253 1255 +2
Misses 178 178
- Partials 97 98 +1 ☔ View full report in Codecov by Sentry. |
seratch
added
enhancement
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thank you so much again! This is indeed an existing bug and we should have fixed it in the past. Left a comment to ask a minor change.
src/receivers/HTTPModuleFunctions.ts
Outdated
| // Check if the response has already been sent | ||
| if (!response.headersSent) { | ||
| // If not, set the status code and end the response to close the connection | ||
| response.writeHead(503); // Service Unavailable |
There was a problem hiding this comment.
Bolt for Python/Java handles this pattern properly but they respond with 404 status. You may have a different view on the status code here but could you amend it to align with them? https://github.com/slackapi/bolt-python/blob/v1.18.1/slack_bolt/app/app.py#L565-L566
| response.writeHead(503); // Service Unavailable | |
| response.writeHead(404); // Not Found |
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Looks great to me! You're a fantastic contributor 👏
|
@seratch Thank you for reviewing the PRs. When would the 2 fixes be released by your team? |
|
@shubhamjajoo Since the next release milestone does not have any remaining tasks, we can release a new version soon. I will ask my team mates if they can make a release later today in North American timezone. If they don't have the bandwidth by the end of Friday, I will make a release next monday in Japan Standard Time (+09:00). |
2 participants
Summary
This pull request fixes an issue where the
defaultUnhandledRequestHandlerdoes not close the HTTP response if an incoming event times out without being acknowledged. The current behavior can lead to open connections that consume server resources unnecessarily and may cause confusion for clients awaiting a response.The proposed change ensures that the server responds with a 503 Service Unavailable status code and ends the response properly, signaling to the client that the event was not processed in time. This update enhances the robustness of the application by preventing potential resource leaks and clarifying the outcome of timed-out events.
Requirements (place an
xin each[ ])