Introduce new query event to elegantly mix evented and callback-style code

[dimsmol]

Currently query always emits end even if error was emitted before, and end handler doesn't know what error occurred if there was one.
It leads to bad looking code if you want to continue with callback-style:

var error;
query
    .on('row', function (row) {
        // row handling code
    })
    .on('error', function (err) {
        error = err;
    })
    .on('end', function (result) {
        callback(error, result); // must be called once wether we got an error or not
    });

It would be great to have some special event to use when we want to continue with a callback:

query
    .on('row', function (row) {
        // row handling code
    })
    .on('next', callback); // expects emit('next', error) on error and emit('next', null, _result) on success

Just using a callback-style is not a solution, because currently it implies rows collecting that is not what we could want. An option to prevent rows collecting could be useful as separate feature, but attempt to solve the problem this way looks less elegant, because we still need row handling code and it will follow callback code in confusing order:

client.query(config_that_prevents_rows_collecting, function (err, result) {
    // callback code
}).on('row', function (row) {
    // row handling code confusingly follows callback code
});

[dimsmol]

There are few other options I thought about, but all of them look less attractive than described above.
Here they are:

• Provide error information in an end event (has unnatural order of result and error args):

query
    .on('row', function (row) {
        // row handling code
    })
    .on('end', function (result, error) { // function introduced just to reverse arguments
        callback(error, result);
    });

• Introduce error property as part of query object itself (requires error unwrapping for callback):

query
    .on('row', function (row) {
        // row handling code
    })
    .on('end', function (result) { // function introduced just to unwrap error
        callback(query.error, result);
    });

• Introduce an event that will be emitted on success only (too verbose):

query
    .on('row', function (row) {
        // row handling code
    })
    .on('error', function (error) {
        callback(error);
    })
    .on('result', function (result) {
        callback(null, result); // won't be emitted if error occurred
    });

[chowey]

Does the end event give a "result" argument like you show? I didn't think it did.

I agree with you that it is currently awkward when both error and end are emitted. Wouldn't it be better just to not emit end if error occurs? It is not backwards compatible, but it is the most straightforward and what I would expect to see.

[dimsmol]

Does the end event give a "result" argument like you show? I didn't think it did.

Yes, it does

Wouldn't it be better just to not emit end if error occurs?

Don't think so. end behaves just like finally clause for query and it may be convenient if you need some cleanup like this:

query.on('end', function () {
    client.end();
});

[brianc]

I think long-term it would be nice for the pg.Query object to implement the interface of the node ReadStream.

[dimsmol]

Following standard interface would be nice.

However, Stream strategy is not to emit end after error and to do cleanup after an error automatically. It has sense because stream becomes completely unusable after an error.

But pg client can still be used if we have db error and are within transaction with savepoint or something. So exactly same strategy is not applicable. An option is to follow Stream interface, but still require manual cleanup (call of client.end()) on error when needed. This variant looks pretty straight and only sad fact about it is that it's not backward compatible.

[dimsmol]

Also, take a look at error handling implementation of Stream, please. It appears that Stream manually throws an exception if there are no listeners on error, so your assumption that unhandled error event will bubble up automatically may prove to be wrong.

[dimsmol]

Found couple of tricks that can help deal with the issue until some resolution will be available.

• easier switching back to callback-style

query
  .on('error', cb)
  .on('end', function (result) {
    if (result) // result is undefined if we got an error
    {
      cb(null, result);
    }
  });

• callback-style cleanup

// helper function
function execute(client, func, cb) {
  // substitue callback with one that will close client and then call original one
  func(client, function (err, result) {
    client.end();
    cb(err, result);
  });
}

// usage
execute(client, function (client, cb) {
  // any client usage, then call cb as usual
  // client.end() will be called in cb
}, cb);

[brianc]

Sorry - it got pretty late for me last night & I wasn't really giving the best answers. First of all, you're right in saying implementing the stream interface isn't the way to go. Secondly, the 'end' callback should have a result object sometime soon. Postgres returns an "end of query" type of message which contains number of rows affected and so on. This should be made available to the end callback. I'll take a look at this.

[dimsmol]

end already has a result object, here is the code:
https://github.com/brianc/node-postgres/blob/master/lib/query.js#L76

[chowey]

I first thought that the end event was more like a success event until experience taught me otherwise.

I propose we just add a success event, which complements the error event. Then we do this:

query
    .on('row', function (row) {
        // row handling code
    })
    .on('error', function (err) {
        callback(err);
    })
    .on('success', function (result) {
        callback(null, result);
    });
    .on('end', function () {
        client.end();
    });

This avoids having to scope a var error=null in some kind of closure around our query call.

This has the advantage that it is backwards compatible, since end is still always called (behaves identically to its current implementation). The original code that @dimsmol posted would still work, for example. But the success event provides cleaner program flow for those who desire it.

[brianc]

makes sense to me and double ❤️ for backwards compat

[chowey]

This has to be the easiest feature to add in the universe.

[chowey]

Is this still an issue? It seems to me that for evented style, use:

client.query(queryString)
  .on('row', function (row) {
    // do some stuff
  })
  .on('error', callback)
  .on('end', function (result) {
    if (result)
      callback(null, result);
  });

and for callback style, just use (of course):

client.query(queryString, callback);