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
set a timeout to get the return from requesting SSL upgrade. #2572
Conversation
@@ -532,6 +532,9 @@ private PGStream enableSSL(PGStream pgStream, SslMode sslMode, Properties info, | |||
|
|||
LOGGER.log(Level.FINEST, " FE=> SSLRequest"); | |||
|
|||
// set the time out to 2seconds | |||
int currentTimeout = pgStream.getSocket().getSoTimeout(); | |||
pgStream.getSocket().setSoTimeout(2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if 2sec would give us flaky timeout errors caused by gc pauses or something else.
Should we have a property to make it configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if 2sec would give us flaky timeout errors caused by gc pauses or something else.
One of those questions only experience will answer. Probably a little too short thoughShould we have a property to make it configurable?
Probably...
docs/documentation/head/connect.md
Outdated
detecting network problems. The timeout is specified in seconds and a | ||
value of zero means that it is disabled. | ||
|
||
* **sslResponseTikmeout** = int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Tikmeout/Timeout/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also bunch of unrelated whitespace conversion of tabs/spaces to this and README. Guessing it's from an editor autosave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh...
LGTM. Not sure what the one AppVeyor failure is as the logs don't show what failed. The other AppVeyor build and rest of CI is good so I say go ahead and squash / merge it. |
* Revert "fix: replace syncronization in Connection.close with compareAndSet" * Revert "feat: synchronize statement executions (e.g. avoid deadlock when Connection.isValid is executed from concurrent threads)" * Revert "set a timeout to get the return from requesting SSL upgrade. (#2572)" * Revert "add alias to the generated query for clarity (#2553)"
…pgrade. (pgjdbc#2572)"" This reverts commit 6b10d8d.
* Revert "Revert "add alias to the generated query for clarity (#2553)"" This reverts commit 6c30e48. * Revert "Revert "set a timeout to get the return from requesting SSL upgrade. (#2572)"" This reverts commit 6b10d8d. * Revert "Revert "feat: synchronize statement executions (e.g. avoid deadlock when Connection.isValid is executed from concurrent threads)"" This reverts commit 50cee9d. * Revert "Revert "fix: replace syncronization in Connection.close with compareAndSet"" This reverts commit 54fa61d.
There are reported instances of cloud services changing the host underneath the connection and hanging when asking for SSL upgrade. This sets the timeout temporarily to avoid this