isValid() timeout should not be blocked

[hugomiguelabreu]

The usage of setQueryTimeout(); with the same value as the setNetworkTimeout(); is blocking the current transaction timeout.
The timeouts are blocking each other with this approach.

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Does ./gradlew autostyleCheck checkstyleAll pass ?
3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

• Does this break existing behaviour? If so please explain.
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully run tests with your changes locally?

[davecramer]

how does this work ? Why does setting the time out there block the other one ?

[hugomiguelabreu]

Hey @davecramer.
From my thread dump and slight investigation, by using the setQueryTimeout() when the database is down, when calling isValid() you'll have a thread trying to kill the task using the killTimerTask(); (line 1022 - PgStatement) because the connection timed out as expected, but not being able to because the connection is stuck trying to cancel it connection.cancelQuery();(line 908 - PgStatement).

Ultimately the synchronized block doesn't allow the killTimerTask(); proceed since the connection.cancelQuery(); is using the connection and is stuck trying to cancel it, since when you cancel a query you are expecting to receive a cancelStream.receiveEOF(); that will never be fulfilled since the database is down.

[davecramer]

got it, thx

[davecramer]

I am wondering if we can create a test for this ?

[hugomiguelabreu]

I am wondering if we can create a test for this ?

Sure we can. Let me get a bit more context on the tests on the repo and soon I'll send a new one!

[davecramer]

Awesome, thanks!

[codecov-io]

Codecov Report

Merging #1943 into master will increase coverage by 4.03%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1943      +/-   ##
============================================
+ Coverage     65.35%   69.39%   +4.03%     
- Complexity     3955     4228     +273     
============================================
  Files           197      197              
  Lines         18006    18004       -2     
  Branches       2919     2919              
============================================
+ Hits          11768    12494     +726     
+ Misses         4878     4158     -720     
+ Partials       1360     1352       -8     

[hugomiguelabreu]

I've coded a regression test to make sure that isValid() respects the timeout.
I've written some javadoc although I think other test don't have it. I feel that it helps a lot to understand what the test is doing.

[vlsi]

@hugomiguelabreu , do you think #898 / https://github.com/NetCrusherOrg/netcrusher-java / https://github.com/trekawek/toxiproxy-java would be helpful here?

[hugomiguelabreu]

It can be helpful for this specific test, nevertheless it's for this specific case, a more overview of the project tests should be analyzed.

[davecramer]

@vlsi we should probably backpatch this.