jshttp / on-finished Goto Github PK

Using on-finished on a request that is then upgraded causes it to erroneously detect that the request is finished.

Below is a minimal example which shows this failure:

var bouncy = require('bouncy');
var on_finished = require('on-finished');

// receiving server that we will proxy to
var engine = require('engine.io');
var server = engine.listen(8080);
server.on('connection', function(socket){
    socket.send('utf 8 string');
});

// proxy server
var server = bouncy(function (req, res, bounce) {
    on_finished(req, function() {
        console.log('finished');
    });

    bounce(8080);
});
server.listen(8000, function() {
    var socket = require('engine.io-client')('ws://localhost:8000', {
        transports: ['websocket']
    });
    socket.on('open', function(){
        console.log('open');
    });
    socket.on('message', function(data) {
        console.log(data);
    });
    socket.on('error', function(err) {
        console.log('error', err);
    });
});

npm install bouncy engine.io engine.io-client

Bouncy is used here since it handles both regular requests and upgrade requests. And engine.io has been hard pinned to use on the websocket transport (same as using raw websockets basically).

Do you mind if I make this library node.js 0.8 compatible? The only problem is the setImmediate, which I would just make fallback to process.nextTick if not there.

In the Readme file, there is a grammatical error. There is redundant for at the last line of paragraph https://github.com/jshttp/on-finished#http-connect-method.

so i remember what the case was. basically if you have something like this:

http.createServer(function (req, res) {
  setTimeout(function () {
    onFinished(res, function () {})
  }, 1000)
})

and the client aborts within that 1 second, it'll crash because node sets res.socket = null somewhere there. or this might actually be a spdy push thing since you can push outside of a req/res loop. hmmm

This module needs real tests, i.e. tests that use http and net. Right now the object is just mocked, so there is no way to know if it actually, really, functions as intended on real objects.

Looking at the implementation in NodeJS.

finished only means that end() has been called on the response object, not necessarily that it actually has "finished", i.e. all data is not guaranteed to have been sent nor is it guaranteed that no further errors will occur...

I think the definition of finished in Node is a little bit confusing. None the less, the semantics are well defined and I don't think the match the assumptions of this library...

It would be useful if the callbacks were called in LIFO order instead of FIFO.

e.g.

const fd = await fs.open(filePath)
onFinished(res, () => fs.close(fd))
await fs.write('test')
onFinished(res, () => fs.fsync(fd)

It would be nice if the callback would get not only error but the actual request object as well on the callback, like so...

onFinished(req, function(err, req) {
    if(err) { return console.error('ERROR:', err); }
    console.log(req);
});

...so that when you are firing multiple requests asynchronously through one handler you could still tell the completing requests apart, fe. for logging purposes.

This lib is very similar to the NodeJS stream.finished() function to me. If I'm using NodeJS version 10 or greater, is there any real advantage of using this lib? Thanks for your time.

https://nodejs.org/api/stream.html#stream_stream_finished_stream_options_callback

and use res or something instead. problem before was that the socket emitted ECONNRESET errors but didn't propagate them to res. not sure if it still does that.

This module is supposed to determine when a response is finished; it should really only accept an actual res as the argument, rather than a socket OR a res OR some object that has a res property.

Koa v2 example

'use string'

var Koa = require('koa')
var app = new Koa()

app.use(({ res }) => {
  res.statusCode = 200
  res.end(JSON.stringify({ hello: 'world' }))
})

app.listen(3000)

autocannon -c 100 -d 5 -p 10 localhost:3000

    at listener (node_modules/on-finished/index.js:170:15)
    at onFinish (node_modules/on-finished/index.js:101:5)
    at callback (node_modules/ee-first/index.js:55:10)
    at Socket.onevent (node_modules/ee-first/index.js:93:5)
    at emitOne (events.js:101:20)
    at Socket.emit (events.js:188:7)
    at onwriteError (_stream_writable.js:340:10)
    at onwrite (_stream_writable.js:358:5)
    at WritableState.onwrite (_stream_writable.js:89:5)

I just Have one problem, I don't understand after I call on-finished function i don't get response body, now my requirement was not to use response.end or response.write function, or to manage events just call on-finished, after i would be able to receive data in response.body.

Is it possible that on-finished manages the body placement?
Is it a good way to hook response body when we are using on-finished function?

may be something is missing for me here, if yes then i apologize.

I entered an issue on a downstream project joelabair/multer-autoreap#14 But it appears that the problem comes from this lib.

sparking this event on socket events rather than solely on the response events is surprising to me.

It poses a problem when I run selenium tests because they somehow closes the socket way before the request is finished.

If this is the expected behaviour of the lib, well could I propose MR where I add the possibility to customize what I'm listening on?

I noticed that the same issue fixed here occurs when a client is pipelining requests to a node server using on-finished for response messages. The pipelined requests cause the response to have the same socket instance, which on-finished is repeatedly attaching new handlers to.

Would a PR adding tests for this case and implementing the same approach (queueing listeners) be considered?

Seems like the current master branch have some broken tests when running Node@8 and Node@9. @UlisesGascon noticed that when running my changes for PR jshttp/mime-db#316

I can not provide a failed build because the workflow is waiting for approval

But I can see the ci-test action failing in my forked repo
https://github.com/carpasse/on-finished/actions/runs/8433556449

Maybe we should have some tests against http2 compat? Also, I think some optimizations can be performed for the http2 case.

So apparently there was a false assumption on the already-finished detection of OutgoingMessage that the socket property will always be populated. From https://github.com/joyent/node/blob/v0.10.32/lib/http.js#L455 in 0.10 (0.8 is similar), socket is not assigned to OutgoingMessage until a later time in the cycle. I believe this occurs with pipelined requests, from reading the Node.js source code, which would explain why it's rarely seen (most things have pipelining turned off and doing things like putting Node.js behind nginx will prevent pipelining from reaching Node.js).

It would be great if the calling order was LIFO (same as stack unwinding) instead of, how it is now, FIFO.

It could easily be accomplished by replacing push with unshift.

When the client side timeouts, the client-side connection is closed. self.emit('close', isException); is executed. ee-first\index.js will issue done(err, ee, event, args) with event === "close". However function onFinish(error) { only captures the "error" which is always null.

今早起来服务起不来了，发现 报了read bind of undefined 错误 一看发现更新用了 async hooks

HTTP2 requests may share one socket, but looking at the code listeners queue is maintained per msg

This causes bunch of error, closed listeners attached to the same socket: code link

Currently onFinished behaves like this:

const onFinished = require('on-finished');
const app = require('express')();

app.use((req, res, next) => {
    const result = onFinished(res, () => /* code */);
    result === res; // true - just not very useful
    next();
});

What could be more useful is:

const onFinished = require('on-finished');
const app = require('express')();

app.use((req, res, next) => {
    const stopListening = onFinished(res, () => /* code */);

    doSomeOperation(err => {
        if (err) {
            stopListening();
            res.sendStatus(500); // onFinished listener is not called
        } else {
            next();
        }
    });
});

My actual use case is a little more complicated, (e.g. I can't just start listening if there isn't an error) and I know you could do this with a flag, but I'd argue in the case where you might have lots of onFinished listeners that need to stop listening, it's cleaner to give them a way to clean themselves up instead of just sitting in memory.

This module is passing a err to the finished callback, but I'm not sure if this even makes sense. Because of the order of events emitted in core, even when an error does occur, the finished callback will be invoked prior with other events and not even pass back an err in most cases. Users who actually care if an err occurred should probably just add an error listener themselves, don't think think?

/cc @jonathanong

AssertionError: non-error thrown: false
    at Object.module.exports.onerror (/Users/tj/dev/koajs/koa/lib/context.js:92:5)
    at Socket.done (/Users/tj/dev/koajs/koa/node_modules/finished/index.js:15:5)
    at Socket.EventEmitter.emit (events.js:126:20)

on "close", because node is full of win

Would be useful to be able to unregister a listener

Looks like error event trigger has changed on new version of node .

Seems like the current master branch have some broken tests when running Node@8 and Node@9.

Issue is similar to jshttp/http-errors#109

I can not provide a failed build because the workflow is waiting for approval

But I can see the ci-test action failing in my forked repo
https://github.com/carpasse/on-finished/actions/runs/8433556449

When I use express to send some file via:

onFinished(res, (err, res) => {
  console.log('transfer done'); // will fire before file sent to user
});

res.sendFile(filepath);

Callback will be fired before file was sent. How can I handle sendFile with on-finished?