End stream after end connection

[wadrian12]

This should solve issue 1344.

[Magellol]

We're facing the ECONNRESET quite often but I can't really tell what is happening exactly.
After reading some of the issues, it seems that this PR might actually solve these, wondering if we could re-open this PR and re-evaluate if it's needed?

[charmander]

@Magellol On pg 7.4.1?

[Magellol]

We're running pg 7.4.0. Was there anything in 7.4.1 that might have an impact?

EDIT: Ah I just checked out the commits and I see that 4cf67b2 might help us. Will upgrade and report back. Thanks

[ghost]

We've been having issues with Azure PostgreSQL keeping connections around (i.e. connection.end() is being called, but the TCP connection is never closed by the server). At some point, the max number of open sockets is reached. Closing the TCP stream after sending 0x58 reliably fixes those issues.

Is there any reason not to close the TCP connection from the client upon sending 0x58? I checked the libpq source code, and it looks like they are closing the TCP connection after sending 'X'. Same for npgsql (.NET library).

[Elexy]

I am running into this issue on managed PG on Azure with ssl as well (i.c.w. postgrator).
When I add the .end() all starts working.

Connection.prototype.end = function () {
  // 0x58 = 'X'
  this.writer.add(emptyBuffer)
  this._ending = true
  this.stream.write(END_BUFFER)
  return this.stream.end()
}

[ghost]

@charmander Is there a reason not to merge this PR? Other libraries seem to behave this way. Or is there's any reason why terminating the stream from the client end is a bad idea?

[sehrope]

I think the socket should be closed. The Postgres docs suggest clients do exactly that after sending a final termination message: https://www.postgresql.org/docs/current/static/protocol-flow.html#id-1.10.5.7.10

The normal, graceful termination procedure is that the frontend sends a Terminate message and immediately closes the connection. On receipt of this message, the backend closes the connection and terminates.

I haven't reviewed the PR in total to see if there's somewhere else that is expecting to be able to close the connection but the overall idea seems sound.

[Magellol]

From my previous #1386 (comment) this ECONNRESET error is still happening after upgrading to [email protected]

I'm wondering if closing the socket would actually solve this issue on our end.

[charmander]

@sheepcount I think it’s a good idea. #1608 makes me a bit uncomfortable right now, though, because it’s pretty much a delayed this.stream.end() and that feels like it could break something even though it doesn’t break the tests. (But maybe not – maybe everything but the initial connection process checks for this._ending already. Still looking at it.)

[ghost]

@charmander I think delaying stream.end() until the asynchronous callback from write() is indeed unnecessary as end() can write END_BUFFER itself. So the simplest change would be to replace this.stream.write(END_BUFFER) with this.stream.end(END_BUFFER).

--- a/lib/connection.js
+++ b/lib/connection.js
@@ -309,7 +309,7 @@ Connection.prototype.end = function () {
   // 0x58 = 'X'
   this.writer.add(emptyBuffer)
   this._ending = true
-  return this.stream.write(END_BUFFER)
+  return this.stream.end(END_BUFFER)
 }
 
 Connection.prototype.close = function (msg, more) {

[Vratislav]

@sheepcount, @charmander good point, my stream kung-fu is not as strong, so apologies for not the most optimal solution in #1608

I quickly tried solution proposed by @sheepcount, but that breaks https://github.com/brianc/node-postgres/blob/master/test/integration/client/quick-disconnect-tests.js

(the modification is in https://github.com/brianc/node-postgres/compare/master...Vratislav:end-stream-write-in-end?expand=1)

[charmander]

@sheepcount Sorry, I forgot to clarify:

maybe everything but the initial connection process checks for this._ending already

Parts of the connection process don’t check for this._ending and will break if the stream ends before the startup messages are sent.