nodeca / probe-image-size

Get image size without full download. Supported image types: JPG, GIF, PNG, WebP, BMP, TIFF, SVG, PSD, ICO.
MIT License
978 stars 77 forks source link

EPIPE or freeze when using probe in http server with keepalive #55

Closed misos1 closed 3 years ago

misos1 commented 3 years ago
let http = require("http");
let probe = require("probe-image-size");

let server = http.createServer(async function(req, res)
{
    res.end(JSON.stringify(await probe(req)));
});

(async function()
{
    await new Promise(resolve => server.listen(8888, resolve));
    let agent = new http.Agent({ keepAlive: true });
    let svg = `<?xml version="1.0" encoding="UTF-8"?><svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="100pt" height="100pt" viewBox="0 0 100 100" version="1.1">`;
    for(let i = 0; i < 100000; i++) svg += " ";
    for(let i = 0; ; i++)
    {
        console.log(i);
        let req = http.request({ port: 8888, method: "post", agent }).end(svg);
        let res = await new Promise((resolve, reject) => req.on("response", resolve).on("error", reject));
        let str = "";
        for await (let chunk of res) str += chunk;
        console.log(str);
    }
}());

Possible output (when is req closed by probe then is sometimes not possible to send response):

0
{"width":100,"height":100,"type":"svg","mime":"image/svg+xml","wUnits":"pt","hUnits":"pt"}
1
node:internal/process/promises:245
          triggerUncaughtException(err, true /* fromPromise */);
          ^

Error: write EPIPE
    at afterWriteDispatched (node:internal/stream_base_commons:160:15)
    at writevGeneric (node:internal/stream_base_commons:143:3)
    at Socket._writeGeneric (node:net:771:11)
    at Socket._writev (node:net:780:8)
    at doWrite (node:internal/streams/writable:412:12)
    at clearBuffer (node:internal/streams/writable:565:5)
    at Socket.Writable.uncork (node:internal/streams/writable:354:7)
    at ClientRequest._flushOutput (node:_http_outgoing:949:10)
    at ClientRequest._flush (node:_http_outgoing:918:22)
    at onSocketNT (node:_http_client:835:9) {
  errno: -32,
  code: 'EPIPE',
  syscall: 'write'
}

When is server part changed to this then it freezes after some time (sending response does not dump partially read request anymore https://github.com/nodejs/node/commit/3d480dcf4c270cbf5a3acef08fc6b8d25576411b):

let server = http.createServer(async function(req, res)
{
    res.end(JSON.stringify(await probe(req, true)));
});

So neither true or false keepOpen does help here. Maybe would be good to have another option which will cause that source stream will be read (and ignored) to end after is probe successful. Or maybe it should do this whether it is successful or encountered error so also in case of error can be send meaningful error response to client.

Maybe keepOpen set to true should actually mean (and be renamed to something else like dontClose or dontDestroy) that stream is not closed but read to end (dumped). Because stream which is not read to end or closed is really bad situation, for file stream this means leak, for http stream leak and possibly other problems as mentioned above. What is user supposed to do with partially read stream anyway? (Ok one possible use case is when is stream piped to multiple destinations, but I think there should be also option to read it to end by probe itself which covers also this situation anyway.)

puzrin commented 3 years ago
  1. Any client can close connection anytime. Issues of server side implementation should not be addressed to this package.
  2. This package is modular. You can create your own http wrapper as you wish.
misos1 commented 3 years ago

Ok, you are probably right. It can be simply solved through finally handler:

let { finished } = require("stream/promises");

let server = http.createServer(async function(req, res)
{
    res.end(JSON.stringify(await probe(req, true).finally(() => finished(req.resume()))));
});