test to reproduce behavior of issue brianc/node-postgres#549 #572

Merged
merged 2 commits into from Apr 24, 2014

Projects

None yet

2 participants

@rngadam
rngadam commented Apr 18, 2014

for brianc/node-postgres#549

With some console.log, the output looks like:

$ node test/integration/client/query-error-handling-prepared-statement-tests.js 
query-error-handling-prepared-statement-tests.js.handleError
lib/query.js isPreparedStatement -> connection.sync
lib/query.js callback
client.end()
lib/client.js client.end
lib/connection.js Connection.end
lib/query.js callback return

copy of query-error-handling-tests.js
a fix was provided in 5079c1e

I would suggest that I provide additional CLs to refactor killIdleQuery in test-helper to reduce duplicated code.

@brianc
Owner
brianc commented Apr 22, 2014

It's okay with the duplicated code, I don't mind. 😄 Duplicated is better than none!

So...this tests passes in its current form, but it's supposed to fail to demonstrate a bug that's fixed in #549 ? Not sure exactly how or why it's passing before the fix has been merged. I'm probably missing what's actually going on here...if you want to submit a failing test to indicate the behavior and then have me run it with the #549 pull request to verify it's fixed I'm cool with it.

@rngadam
rngadam commented Apr 23, 2014

hmm, you're right, I was actually testing a different path. The handleError I wanted to test happens on connection end(), while the previous version of the test covered error connection event.

With the last commit, I've put them both side by side in the test and make sure it fails before and not after:

$ node test/integration/client/query-error-handling-prepared-statement-tests.js
query-error-handling-prepared-statement-tests.js..
$ git revert --no-commit 5079c1e
$ node test/integration/client/query-error-handling-prepared-statement-tests.js
query-error-handling-prepared-statement-tests.js..Message: Cannot call method 'sync' of undefined
TypeError: Cannot call method 'sync' of undefined
    at Query.handleError (/home/rngadam/letsface/src/node-postgres/lib/query.js:90:16)
    at Client.connect (/home/rngadam/letsface/src/node-postgres/lib/client.js:186:24)
    at g (events.js:192:14)
    at EventEmitter.emit (events.js:93:17)
    at Socket.<anonymous> (/home/rngadam/letsface/src/node-postgres/lib/connection.js:66:10)
    at Socket.EventEmitter.emit (events.js:96:17)
    at Socket._destroy.destroyed (net.js:358:10)
    at process.startup.processNextTick.process._tickCallback (node.js:245:9)

 TypeError: Cannot call method 'sync' of undefined
    at Query.handleError (/home/rngadam/letsface/src/node-postgres/lib/query.js:90:16)
    at Client.connect (/home/rngadam/letsface/src/node-postgres/lib/client.js:186:24)
    at g (events.js:192:14)
    at EventEmitter.emit (events.js:93:17)
    at Socket.<anonymous> (/home/rngadam/letsface/src/node-postgres/lib/connection.js:66:10)
    at Socket.EventEmitter.emit (events.js:96:17)
    at Socket._destroy.destroyed (net.js:358:10)
    at process.startup.processNextTick.process._tickCallback (node.js:245:9)

$ git revert --abort
$ node test/integration/client/query-error-handling-prepared-statement-tests.js
query-error-handling-prepared-statement-tests.js..

Is it correct that in neither case the query emits an error event?

@rngadam
rngadam commented Apr 23, 2014

The test would fail when running as native when checking the error string. I think it's better if both are consistent so I've changed it to be the same.

@rngadam
rngadam commented Apr 23, 2014

OK, I need some guidance as the native version differs greatly from the Javascript one. The test just hangs there never calling back. Is it intended that the behavior should be reimplemented in the native version? There's no Connection.sync() to start with, so I'm not sure what work is needed.

@rngadam
rngadam commented Apr 23, 2014

Continuous build tripped on an intermittent failure here:

query-error-handling-prepared-statement-tests.js..Message: read ECONNRESET
Error: read ECONNRESET
    at errnoException (net.js:901:11)
    at TCP.onread (net.js:556:19)
 Error: read ECONNRESET
    at errnoException (net.js:901:11)
    at TCP.onread (net.js:556:19)

I can reproduce by repeatedly calling the test:

while true; do node test/integration/client/query-error-handling-prepared-statement-tests.js ; done

Updated to make it work as per:

  this.stream.on('error', function(error) {
    //don't raise ECONNRESET errors - they can & should be ignored
    //during disconnect
    if(self._ending && error.code == 'ECONNRESET') {
      return;
    }
    self.emit('error', error);
  });

I found it questionable that we would set the state _ending after calling _send so I moved it before calling _send.

@brianc
Owner
brianc commented Apr 23, 2014

Where does the test hang on the native version?

@rngadam
rngadam commented Apr 24, 2014

The sleep query never calls the callback. I've confirmed the killIdleQuery goes through ok.

Ricky Ng-Adam added some commits Apr 18, 2014
Ricky Ng-Adam test to reproduce behavior of issue brianc/node-postgres#549
a fix was provided in 5079c1e;
test is modeled on query-error-handling-tests.js;
test both kill query and disconnection on prepared statement execution;
make connection error string message consistent between native and non-native;
disable test server-side kill for native as it hangs;
sync can cause error to be emitted so we catch that;
we also move _ending state before _send is called.
6216f60
Ricky Ng-Adam capture error message from intermittent failure of copy-tests.js fbedaf4
@brianc
Owner
brianc commented Apr 24, 2014

Thank you so much for your help and work on this. Means a lot to me! I'm like hair on 🔥 at work right now so it took me longer than I should have to get to this...but it's much appreciated. 👍

@brianc brianc merged commit 4c8f489 into brianc:master Apr 24, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@sentientwaffle sentientwaffle added a commit to Voxer/zag that referenced this pull request Jun 2, 2014
@sentientwaffle sentientwaffle backend-pg: bump "pg" to latest version
Trying to fix:

    TypeError: Cannot call method 'sync' of undefined
      at Query.handleError (/voxer/deploy/server/node_modules/zag-backend-pg/node_modules/pg/lib/query.js:92:16)
      at Client.connect (/voxer/deploy/server/node_modules/zag-backend-pg/node_modules/pg/lib/client.js:184:24)
      at g (events.js:192:14)
      at EventEmitter.emit (events.js:93:17)
      at Socket.<anonymous> (/voxer/deploy/server/node_modules/zag-backend-pg/node_modules/pg/lib/connection.js:60:10)
      at Socket.EventEmitter.emit (events.js:93:17)
      at TCP.onread (net.js:423:51)

via brianc/node-postgres#572.
9ff9936
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment