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

ERROR: aws-serverless-express connection error on DELETE request #106

Closed
cliffgibson opened this issue Nov 3, 2017 · 17 comments
Closed
Assignees
Labels

Comments

@cliffgibson
Copy link

I just follow the example to wrap my Express application. There is a "app.delete" handler in my app. When DELETE request is being served, there is strange error thrown out from aws-serverless-express, as shown below

ERROR: aws-serverless-express connection error
{ Error: socket hang up at createHangUpError (_http_client.js:253:15) at Socket.socketOnEnd (_http_client.js:345:23)

My code is very simple:

app.delete('/user', routes.user.remove);
module.exports.remove = [
(request, response) => {
let user = request.body;
console.log('remove user='+JSON.stringify(user));
db.users.delete(user.username, (err) => {
if (err) {
response.status(500).send('internal error');
}
else {
response.json({ username: user.username });
}
});
}
];

Just for testing purpose, if I replace the app.delete handler with app.patch, and invoke the call using HTTP PATCH method, with exactly same request parameters, the call is handled correctly. And this is my workaround for the moment.

Pls help. Thank you

@ro-tex
Copy link

ro-tex commented Jan 31, 2018

I have the same issue when I send a body with DELETE requests.

There is an interesting discussion about whether a body is allowed here: https://stackoverflow.com/questions/299628/is-an-entity-body-allowed-for-an-http-delete-request

The conclusion seems to be that the body should be allowed and ignored. In any event, causing a 502 doesn't seem like the best possible approach.

@brettstack
Copy link
Collaborator

@cliffgibson please confirm: did the DELETE request have a body? I will try to reproduce.

@brettstack brettstack self-assigned this Apr 10, 2018
@novemberde
Copy link

@cliffgibson DELETE request is not supported response body. Please use POST or PUT method instead of DELETE If you really want to get a response body.

@ro-tex
Copy link

ro-tex commented Apr 11, 2018

@novemberde the issue is not that we want to receive a body. We want to be able to send a body without getting a 502 error. The RFC allows for that.

@brettstack
Copy link
Collaborator

We're not doing anything funny in this library checking for a body on DELETE and causing a 502. I'll look into it and see where the problem lies. I assume this doesn't happen with a standard Express app (i.e. can you run it locally and DELETE works?(

@novemberde
Copy link

@brettstack Yes, DELETE works on Express app.
here's code.
const app = require("express")();
app.delete("/", (req, res, next) => {
res.status(200).json({
message: "Receive body on DELETE Method!"
});
});
app.listen(3000, () => console.log("Running!"));

@SeanLMcCullough
Copy link

Encountered the same issue, and it's especially infuriating when you have the same code executed by PUT and DELETE but DELETE crashes without a single understandable error message. This should be fixed, the RFC HTTP1.1 spec doesn't forbid the use or request body on DELETE commands, and as such - express supports it.

I don't have the time to confirm this, but does API Gateway support request body on DELETE?

@HotelCalifornia
Copy link

Have there been any updates on this?

@HotelCalifornia
Copy link

Also:

I don't have the time to confirm this, but does API Gateway support request body on DELETE?

I've just confirmed that it does. Somewhere along the line it seems that this module is dropping the body for DELETE requests

@brettstack brettstack added the bug label May 30, 2018
HotelCalifornia added a commit to HotelCalifornia/aws-serverless-express that referenced this issue May 30, 2018
- without content-length specified, it seems the body of DELETE requests get ignored
- per RFC 2616, section 4.3: The presence of a message-body in a request is signaled by the inclusion of a Content-Length or
Transfer-Encoding header field in the request's message-headers. A message-body MUST NOT be included in a request if
the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests. A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request.
  - in other words, whether or not it is recommended to have a body in a DELETE request, if the content-length header (or the transfer-encoding header) is set, the message body should be forwarded
- concerns:
  - I'm not sure whether it is necessary to perform more type checking on `event.body`. for instance, if it is an object, it should be passed through a call to `JSON.stringify` before getting the byte length
  - the header should probably be set only if not already specified

This commit is one possible solution to CodeGenieApp#106
@guzmonne
Copy link

guzmonne commented Jun 13, 2018

This error happened to me and the reason was that I was sending a body with the DELETE request. After I took it the error went away. For me this fixes the problem, but as I understand, sending a body along with a DELETE method is not wrong.

@brettstack
Copy link
Collaborator

You can try the PR. I haven't confirmed it though.

@ajstocchetti
Copy link

ajstocchetti commented Jun 19, 2018

I tested @HotelCalifornia's PR and it worked. Until it gets merged in, I was able to get around this by manually setting the content length if its not:

exports.handler = (event, context) => {
  // fix to deal with body being dropped on DELETE requests
  if (event.httpMethod === 'DELETE' && event.body && (!event.headers || !event.headers['Conent-Length'])) {
    console.log('Adding content length for DELETE body');
    event.headers = event.headers || {};
    event.headers['Content-Length'] = Buffer.byteLength(event.body);
  }
  return awsServerlessExpress.proxy(server, event, context);
}

@brettstack
Copy link
Collaborator

I'm working on integ tests right now and will merge that PR after they're in. Is the issue with API Gateway not sending the Content-Length header for DELETE requests with a body?

@HotelCalifornia
Copy link

I think the issue is just that API gateway ignores the body in a DELETE request unless the Content-Length header is specified

@brettstack
Copy link
Collaborator

I don't think that's what's happening. event.body is always coming through if you provide it in the request, right? My understanding is Express/Node.js http is throwing 502 when body exists with no content-length header in the request. So my assumption here is that API Gateway isn't including a content-length header for DELETE requests even when they have a body.

Your CR looks good. I just made a couple comments.

@HotelCalifornia
Copy link

You're right, that does sound more correct. If I'm honest, it's been a few weeks since I've thought about this problem 😅

brettstack added a commit that referenced this issue Jun 25, 2018
* Certain requests (e.g. GET and DELETE) would fail when they included a body due to missing content-length header. See #147, #106, #130
brettstack added a commit that referenced this issue Aug 16, 2018
* Certain requests (e.g. GET and DELETE) would fail when they included a body due to missing content-length header. See #147, #106, #130
@brettstack
Copy link
Collaborator

Closing in favor of #175. This will be out in the next release

brettstack pushed a commit that referenced this issue Aug 20, 2018
<a name="3.3.5"></a>
## [3.3.5](v3.3.4...v3.3.5) (2018-08-20)

### Bug Fixes

* apply Content-Length header when missing ([b0927b8](b0927b8)), closes [#147](#147) [#106](#106) [#130](#130)
* apply Content-Length header when missing ([#175](#175)) ([c2f416b](c2f416b))
OneDev0411 added a commit to OneDev0411/serverless-express that referenced this issue Mar 16, 2023
<a name="3.3.5"></a>
## [3.3.5](CodeGenieApp/serverless-express@v3.3.4...v3.3.5) (2018-08-20)

### Bug Fixes

* apply Content-Length header when missing ([b0927b8](CodeGenieApp/serverless-express@b0927b8)), closes [#147](CodeGenieApp/serverless-express#147) [#106](CodeGenieApp/serverless-express#106) [#130](CodeGenieApp/serverless-express#130)
* apply Content-Length header when missing ([#175](CodeGenieApp/serverless-express#175)) ([c2f416b](CodeGenieApp/serverless-express@c2f416b))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants