Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure isValid() will not last more than timeout seconds #1557

Merged
merged 5 commits into from Sep 5, 2019
Merged

Ensure isValid() will not last more than timeout seconds #1557

merged 5 commits into from Sep 5, 2019

Conversation

@arobert-delfingen
Copy link
Contributor

@arobert-delfingen arobert-delfingen commented Sep 3, 2019

This PR is to just to give an idea how to fix issue #1556, but never tested

even if the underlying socket is broken (issue #1556)
@AppVeyorBot
Copy link

@AppVeyorBot AppVeyorBot commented Sep 3, 2019

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Sep 3, 2019

Makes sense

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Sep 3, 2019

@arobert-delfingen can you rebase and push another commit as the .travis.yml file was broken, thanks

Loading

@AppVeyorBot
Copy link

@AppVeyorBot AppVeyorBot commented Sep 4, 2019

Loading

@AppVeyorBot
Copy link

@AppVeyorBot AppVeyorBot commented Sep 4, 2019

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Sep 4, 2019

@arobert-delfingen so this is now failing most all tests, except for checkstyle ...

Loading

@AppVeyorBot
Copy link

@AppVeyorBot AppVeyorBot commented Sep 5, 2019

Loading

@arobert-delfingen
Copy link
Contributor Author

@arobert-delfingen arobert-delfingen commented Sep 5, 2019

Now all tests succeed, but theoritically I should :

  • Surround the 4 calls to get/setNetworkTimeout() by a try/catch (not only the last)
  • Not catch all kinds of SQLException, this is a little bit too wide, but maybe only SQLException having state PSQLState.CONNECTION_DOES_NOT_EXIST (and maybe also PSQLState.COMMUNICATION_ERROR).
    What do you think about that ?

Loading

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 5, 2019

Codecov Report

Merging #1557 into master will decrease coverage by 0.03%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master    #1557      +/-   ##
============================================
- Coverage     68.92%   68.88%   -0.04%     
  Complexity     3971     3971              
============================================
  Files           179      179              
  Lines         16545    16569      +24     
  Branches       2695     2695              
============================================
+ Hits          11403    11414      +11     
- Misses         3890     3902      +12     
- Partials       1252     1253       +1

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Sep 5, 2019

@arobert-delfingen yes, to all of the above. Thanks!

Loading

@AppVeyorBot
Copy link

@AppVeyorBot AppVeyorBot commented Sep 5, 2019

Loading

@arobert-delfingen
Copy link
Contributor Author

@arobert-delfingen arobert-delfingen commented Sep 5, 2019

Any idea why when writen like this it breaks test ? where is my mistake ?

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Sep 5, 2019

I don't worry too much about Appveyor failures. If it passes Travis I'm good.

Loading

@davecramer davecramer merged commit b2eaefe into pgjdbc:master Sep 5, 2019
1 of 2 checks passed
Loading
@AppVeyorBot
Copy link

@AppVeyorBot AppVeyorBot commented Sep 5, 2019

Loading

rbrw added a commit to puppetlabs/puppetdb that referenced this issue Nov 21, 2019
PostgreSQL JDBC driver has a bug where the isValid() call on a
connection does not respect timeouts. This causes our connection pool to
not detect a dead connection, which will eventually exhaust our
connection pool entirely. See pgjdbc/pgjdbc#1557
rbrw added a commit to puppetlabs/puppetdb that referenced this issue Nov 22, 2019
PostgreSQL JDBC driver has a bug where the isValid() call on a
connection does not respect timeouts. This causes our connection pool to
not detect a dead connection, which will eventually exhaust our
connection pool entirely. See pgjdbc/pgjdbc#1557
davecramer added a commit to davecramer/pgjdbc that referenced this issue Jul 5, 2021
* Ensure isValid() will not last more than timeout seconds
even if the underlying socket is broken (issue pgjdbc#1556)

* Fix check style

* Add a try/catch to succeed tests

* Manages simply and properly get/setNetworkTimeout() in isValid()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants