-
Notifications
You must be signed in to change notification settings - Fork 394
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
ack
in ExpressReceiver
ending response twice
#327
Labels
bug
M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented
Comments
jarrodldavis
changed the title
Dec 6, 2019
ack
in ExpressReceiver
ending request twiceack
in ExpressReceiver
ending response twice
shaydewael
added
the
bug
M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented
label
Dec 6, 2019
@jarrodldavis Good catch, I think you're right about changing it to a if/else if/else statement to future proof it against Express changes (and the logic should be correct anyway 😅) |
stevengill
added a commit
to stevengill/bolt
that referenced
this issue
Jan 11, 2020
2 tasks
stevengill
added a commit
that referenced
this issue
Jan 13, 2020
fixed ack in ExpressReceiver firing twice. Issue #327
seratch
pushed a commit
to seratch/bolt-js
that referenced
this issue
Jan 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
bug
M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented
Description
The
ack
method configured on events emitted byExpressReceiver
attempts to end the request twice for an emptyresponse
value.What type of issue is this? (place an
x
in one of the[ ]
)Requirements (place an
x
in each of the[ ]
)Bug Report
Reproducible in:
package version: 1.4.1
node version: 12.12.0
OS version(s): macOS Catalina, Version 10.15.1 (19B88)
Steps to reproduce:
Expected result:
The bot receives the messages, responds with an acknowledgement without error, and proceeds with the logic of the
App
instance (e.g. usingsay
).Actual result:
With an unaltered Express
response
object, the expected result occurs, but this is only because Express (currently) checks for an existingContent-Type
header in itsRequest#json
method.Attachments:
I've tracked down the issue (and why it currently doesn't blow up on unaltered Express response objects) to this code path:
ExpressReceiver
emits amessage
event in itsrequestHandler
:https://github.com/slackapi/bolt/blob/d2db40b85e80b95b83cf28dc30f63dd4d0e17b47/src/ExpressReceiver.ts#L72-L112
That comes through to
App#onIncomingMessage
, where it immediately acknowledges incoming Events API requests:https://github.com/slackapi/bolt/blob/d2db40b85e80b95b83cf28dc30f63dd4d0e17b47/src/App.ts#L465-L471
ack
is defined here:https://github.com/slackapi/bolt/blob/d2db40b85e80b95b83cf28dc30f63dd4d0e17b47/src/ExpressReceiver.ts#L85-L98
For a falsy (e.g.
undefined
)response
value, it first callsres#send
, followed byres#json
.res#send
, of course, sets appropriate response headers along with the response body and ends it:https://github.com/expressjs/express/blob/e1b45ebd050b6f06aa38cda5aaf0c21708b0c71e/lib/response.js#L107-L225
At this point, no additional headers can be set on the response.
Similarly,
res#json
sets a JSON response along with response headers, and ends the response viares#send
.https://github.com/expressjs/express/blob/e1b45ebd050b6f06aa38cda5aaf0c21708b0c71e/lib/response.js#L239-L268
Thankfully,
res#json
in the current (v4.17.1
) version of Express checks for an existingContent-Type
header and doesn't set any additional headers, in part because the call tores#send
hasundefined
as the argument.So while this currently doesn't pose an issue, any change to how Express handles different values in regards to setting appropriate headers may cause surprising "headers can't be set after being sent to the client" errors in the future.
I believe the fix is to change the
ack
method:But I'm not sure how to write a test for this since there currently isn't a failing test case outside of standing up a non-vanilla Express environment.
Of note, I found this issue due to using Bolt in an API route of Next.js, which modifies response objects to have custom
send
andjson
functions that aren't as judicious in their setting of response headers.The text was updated successfully, but these errors were encountered: