pgjdbc
loginTimeout in java.sql.DriverManager never be used
in the file:pgjdbc/src/main/java/org/postgresql/PGProperty.java,line 246.
LOGIN_TIMEOUT("loginTimeout", "0",
"Specify how long to wait for establishment of a database connection.");
the default value "0" will lead to the result that the value set in DriverManager never be used.because the code using the value as follows: in the file:pgjdbc/pgjdbc/src/main/java/org/postgresql/Driver.java,line 633
private static long timeout(Properties props) {
String timeout = PGProperty.LOGIN_TIMEOUT.get(props);
if (timeout != null) {
try {
return (long) (Float.parseFloat(timeout) * 1000);
} catch (NumberFormatException e) {
LOGGER.log(Level.WARNING, "Couldn't parse loginTimeout value: {0}", timeout);
}
}
return (long) DriverManager.getLoginTimeout() * 1000;
}
and in the method "get" of PGProperty,the code as follows: in the file:pgjdbc/src/main/java/org/postgresql/PGProperty.java,line 475.
public String get(Properties properties) {
return properties.getProperty(_name, _defaultValue);
}
as above,the LOGIN_TIMEOUT will always get the default value of "0",the loginTimeout value set in the DriverManager will never be used. if we want to set the loginTimeout, we must put it into properties,this will make confusion when we develop. The phenomenon will be that the login will hang if the network is error although we have set the loginTimeout value in the DriverManager.
Hi, I want to help out and created a PR #900 for this. The first commit fixed the issue reported here. And then I saw that there is no reason to use Float.parseFloat to parse int value (I assume loginTimeout value should always be integer in secs), so I made 2nd commit to improve this as well. Let me know if that works for you.
why don't you just set the default value to null, it's more easy. Just as follows:
LOGIN_TIMEOUT("loginTimeout", null,
"Specify how long to wait for establishment of a database connection.");
I guess the original author has his concern to make the value precise to milliseconds. So i suggest that you don't change its logical code.
Because having NULL as default for int param seems odd.
If milliseconds were intented then it should take millis as long unit value Instead. I think it's odd to take float as parameter and call it secs.
How about the case that the user set LOGIN_TIMEOUT to "0" while the value in DriverManager is not 0? Which one do you choose? And i suggest you to use Long instead of Integer.
The current fix will use PG conn property if it's NOT zero, else it will use DriverManager. So your case will use second one. The DriverManager also default to zero if you don't set anything.
Note that the DriverMangaer.loginTimeout is documented as secs and is using "int". The PG conn properly "loginTimeout" is also documented as in "secs" as well, and a max java "int" value can hold about 68 years! Is Long really needed for such timeout?
The only concern I have myself, as you already pointed out, is that changing from Float.parseFloat to Integer.parseInt might break existing user config if they already started using decimal number values for loginTime (Well, actually they will get a WARNING msg b/c bad parsing, and then default to DriverManager.loginTimeout() instead). But as I argued, it will make PG conn properly consistent and easier to document from API perspective. So it will be a decision for the PG committers to decide whether they want this improvement or not.
I am happy to make any modification you guys need. Just let me know what's more reasonable.
Why this issue is still open? and The latest version is still has the same problem?
@gongmingbin because we asked the author of the PR to make changes and this has not been done. If you wish you can fix up the PR so we can commit it
Hi Dave, I wasn't aware that there were pending items from me to change further. I think it's just waiting for committer to review and approve the PR I submitted. If there is anything you need, please let me know and I can help put it in.
@zemian see https://github.com/pgjdbc/pgjdbc/pull/900/files for requests for changes
Hi Dave, I think I did address all the requests for changes. See all the replies I made on the "Conversation" tab side. The last reply I made was this: https://github.com/pgjdbc/pgjdbc/pull/900#issuecomment-341902369 regarding test assertion review comments. I thought I did my best and was just waiting for approval. :)
I have now moved on to another project and I don't have much free time. So if something quick, I will be happy to help, otherwise you guys might need to pick up the rest of change.
On Tue, Nov 17, 2020 at 2:08 PM Dave Cramer [email protected] wrote:
@zemian https://github.com/zemian see https://github.com/pgjdbc/pgjdbc/pull/900/files for requests for changes
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pgjdbc/pgjdbc/issues/879#issuecomment-729139520, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFJ2JECY7CXFCW55B6352YLSQLC2DANCNFSM4DUB3BZQ .
Hi, I come from 5 years in the future following a 1 hour long production outage in my services due to this bug... (I was using Driver-mode Hikari and loginTimeout settings from it weren't being respected). Any chance we can get some movement here?
