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

Close HTTP response on unhandled request timeout #2007

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

suhailgupta03
Copy link
Contributor

Summary

This pull request fixes an issue where the defaultUnhandledRequestHandler does 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 x in each [ ])

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.
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (5587d01) 82.00% compared to head (b42e6f7) 81.97%.

Files Patch % Lines
src/receivers/HTTPModuleFunctions.ts 75.00% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@suhailgupta03 suhailgupta03 marked this pull request as ready for review December 1, 2023 08:20
@seratch seratch added enhancement M-T: A feature request for new functionality semver:major semver:patch and removed semver:major labels Dec 1, 2023
@seratch seratch added this to the 3.16.0 milestone Dec 1, 2023
@seratch seratch self-assigned this Dec 1, 2023
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

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.

// 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
response.writeHead(503); // Service Unavailable
response.writeHead(404); // Not Found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seratch Updated.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Looks great to me! You're a fantastic contributor 👏

@seratch seratch merged commit 0b04722 into slackapi:main Dec 1, 2023
8 checks passed
@suhailgupta03
Copy link
Contributor Author

@seratch Thank you for reviewing the PRs. When would the 2 fixes be released by your team?

@seratch
Copy link
Member

seratch commented Dec 1, 2023

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:signed enhancement M-T: A feature request for new functionality semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants