-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Missing response for certain POST requests #12339
Comments
I suspect it's the same issue here: Other related issues (http upload + EPIPE): A bit of further research reveals also that it used to work in v0.8 & v0.10 (which doesn't fail at all), and that the regression was introduced with v0.12.0, though the error message is slightly different:
Finally, it's possible that this issue could be fixed if #947 is resolved. |
With a slight change to the test ( That means, the last time this worked was on v0.8. |
@jasnell @bnoordhuis Why was nodejs/node-v0.x-archive#7151 abandoned? It still fails on v7. |
@nodejs/http |
I can confirm the behavior but I'm ambivalent on how (or if) it should be fixed. The server sends a response and closes the connection, causing the client to get an EPIPE because it's still busy writing the request data. Ignoring the EPIPE lets it read the response. The request data is lost to the ether, though. Some of it may still have been processed by the server but you don't know how much. @kanongil What do you propose? This seems like a no-win situation. |
@bnoordhuis The server behavior is a perfectly legal way to send a response to a client when the request payload is unnecessary to generate a response. Eg. a
This is irrelevant. If the server has decided to send a response, it doesn't matter how much data actually made it through. The client just needs to handle the response. I don't know how you would go about fixing the issue. I do know, that with the async behavior and internal buffering in streams, it is not reasonable to expect a client to know when to stop writing (to avoid the One idea I had was to defer the |
With all due respect, that seems a little naive; it's a very literal reading of the spec. The real world isn't going to be so simple.
Maybe I didn't make myself clear but the client, whether it's node or something else, cannot reliably know that in advance: the EPIPE error is the operating system telling us the connection was severed. I thought it over for a good while but I'm not optimistic a fix exists that doesn't break more than it fixes. If you manage to come up with something, send a pull request and I'll be happy to review it. |
From https://tools.ietf.org/html/rfc7230#section-6.6, it seems that you are somewhat right. However, this is data that has already been received from the tcp socket, but is lingering in internal buffers. Afaik, it's not really an error, but rather a standard way to end connections. |
I think what you are observing is the
Many HTTP servers trip over this, including nginx (see bug I filed here: https://trac.nginx.org/nginx/ticket/1250#comment:4). TCP does not allow you to As I wrote on the nginx bug (slightly amended to be general beyond nginx):
The correct way for a server to shut down a HTTP/1.1 connection is:
|
Given @nh2's comment, is there a proper way forward in NodeJS to perform these steps in a safe, user-friendly way? We have a case where users could upload files of nearly any size in a browser, and I'd prefer not having to read the entire file server-side only to fail, especially when we can determine the file can't be uploaded from the headers alone. We do have a few alternatives for our use case, but it'd be nice for our API to cleanly short-circuit requests. |
I entered the same issue as #21026 but I get ECONNRESET instead of EPIPE. This is in line with what @nh2 says. If I understand @camlegleiter correctly, he is looking for a Node.JS fix for implementing a server. I'm looking for a way to work around this problem on the client side, as I need to talk to a broken server. So moving forward:
|
@camlegleiter Doens't look like it. You can send
@rogierschouten I'm not aware of any. You'd have to ask all OS vendors to add one. But I think even that wouldn't be enough: If I remember correctly, TCP doesn't guarantee any order of |
This is same in NodeJS v10.13.0 When I use curl for the request, the error never occured. |
This code sometimes throw error (failed), sometimes not (success): const http = require('http')
const port = 8080
// PUT method is not allowed, always get 405
const METHOD = 'PUT'
const upload = function (i) {
let response
const req = http.request({
port: port,
method: METHOD
}, function (res) {
response = res
console.log(i + '. SUCCESS request')
// console.log('received status code:', res.statusCode)
let data = ''
res.setEncoding('utf8')
res.on('data', function (chunk) {
data += chunk
})
res.on('end', function () {
// console.log('received message:', data)
})
res.on('error', function (err) {
console.log('got response error:', err)
})
})
req.on('abort', function (err) {
console.log('got request abort:', err)
})
req.on('error', function (err) {
console.log(i + '. Request FAILED: ' + err.message)
// console.log(err.code)
if (err.code === 'EPIPE') {
if (!response) {
// console.log(err.message)
// console.log('Response is undefined')
}
}
return
})
const buff = Buffer.alloc(10*1024*1024, 'x')
req.end(buff)
}
const server = http.createServer(function (req, res) {
if (req.method !== 'POST') {
// never reading data
res.statusCode = 405
res.end('Allowed method is POST')
} else {
let length = 0
req.on('data', chunk => {
length += chunk.toString().length
})
req.on('end', () => {
// console.log('request end, length:', length)
res.end('length is ' + length)
})
}
})
server.listen(port, function (err) {
if (err) {
return console.log('something bad happened', err)
}
console.log('server is listening on', port)
for (let index = 0; index < 10; index++) {
upload(index + 1)
}
}) |
Important, code is never reading request data, when it arrived with PUT method: I was expecting it is always failing. if (req.method !== 'POST') {
// never reading data
res.statusCode = 405
res.end('Allowed method is POST')
} else { |
Some output:
|
Same issue with node v11.10.0 under ubuntu 18.04. As i understand, this happens when readeble trying to pipe to aborted writeble, but 'abort' event does not emitted and node does not handle it. |
It works fine in v12.16.1:
res.end('Allowed method is POST') sends data back, and an EPIPE ✅ |
This error was causing us to hit H18 errors on Heroku - what was happening is that our API was accepting large HTTP bodies in the form of file uploads, and that plus an invalid API key was causing our error handler to return a response before the HTTP body had been fully processed yet. I managed to reliably reproduce in a unit test on my dev machine with something like the following: const req = http.request('http://localhost:3000/api', {
method: 'POST',
});
let errorCode = null;
req.once('error', err => {
// Error code is either EPIPE or ECONNRESET
errorCode = err.code;
});
req.once('close', () => {
expect(errorCode).toBeNull();
});
req.end(Buffer.alloc(10 * 1024 * 1024)); The error was always either I fixed it in my express error handler by waiting for the incoming request to end before sending a response: function (err, req, res, next) {
function respond() {
return out.status(status).send({ error });
}
if (req.complete) return respond();
req.once('data', () => {});
return req.once('end', respond);
} It is completely possible for the request to already be complete by the time it gets to this point, hence the check on |
@bnoordhuis reading through this issue and others I'm unsure what is the conclusion. |
@gsimko So, I think that particular scenario could be fixed (if it doesn't work already) but it may not be easy because node's http client is kind of a tangled mess. The underlying TCP socket can be in a half-open state where reading works but writing fails. I think (but am not 100% sure) the If however that doesn't work, please send fixes. :p |
Possible fix of this issue (eb19b1e) already released with Node.js 18.11.0 |
Thank you @bnoordhuis and @wa-Nadoo! I can confirm that upgrading node helped. I went directly to node 19.1.0 and luckily the upgrade seems to be without hiccups. I was on 18.10.0 before, so there's chance that the fix indeed came in 18.11.0. |
It works fine in Node.js v18.12.1, I think it is closable, because #27916 |
I'm still facing this issue on node 18, and this is the workaround I'm doing on the server side:
Is this really the best way? We use multer for file upload, so if the file extension doesn't match, we want to respond quickly, but then we get the EPIPE on the client as the file is still being transferred. If the file is huge (which we have some customers with large files), are we really supposed to wait for the entire file to be uploaded before responding? I don't see a way to send |
When a server responds with a short payload to a
POST
http.request()
with a large payload that has not finished uploading, a node client throws anEPIPE
error on the request object. E.g. withNODE_DEBUG=http
:This destroys the node socket, and the http client never receives the
response
or any of the payload data.Expected result
The
EPIPE
error should be deferred (or possibly just ignored) until the client has had a chance to handle the response and payload.Serverside workaround
This can be mitigated serverside, by consuming all the uploaded data before sending a response.
Background
I need clients to be able to handle my server responding to
POST
requests without consuming the entire payload.Example
I have created a failing example, which when run will trigger the
EPIPE
error.To visualise that this is indeed a node bug, I added an
ignore_epipe
option, which can be set totrue
, to ignoreEPIPE
errors on the socket. This results in the client behaving as I expected, properly delivering theresponse
and payload, before emitting an error.The text was updated successfully, but these errors were encountered: