loginTimeout is not followed under network partition and clock jump

[stevenliang16]

I'm submitting a ...

• bug report
• feature request

Describe the issue
The driver does not follow the login timeout when there is clock jump on client and network partition between client and server. We have set loginTimeout to 10 seconds and socketTimeout to 20 seconds, but we have seen some queries hanging for more than 1 hour.

Driver Version?
42.0.0

Java Version?
1.8.0_191

OS Version?
Ubuntu 16.04

PostgreSQL Version?
11.2

Analysis
There are two bugs that together cause the issue

1. Use of non-monotonic time System.currentTimeMillis()
   https://github.com/pgjdbc/pgjdbc/blob/REL42.0.0/pgjdbc/src/main/java/org/postgresql/Driver.java#L364-L412
   Login timeout is calculated with System.currentTimeMillis(), which is not monotonic. If there is a backward clockjump after the calculation of expiry and before the calculation of delay, it is possible that delay has a very large value, which is then passed to wait() method (on line 400).
   This wait() method will wait until either another thread invokes the notify() method or the notifyAll() method for this object, or a specified amount of time has elapsed.

2. No socket timeout set for socket read in enableSSL
   When making a connection, the separate thread will call ConnectionFactoryImpl.openConnectionImpl to open a connection. Inside the function, it calls enableSSL to send ssl startup packet and enable ssl for an established connection. However, this step does not have socket timeout set, which means it can get stuck forever.

3. How these two bugs cause the issue
   Due to the first bug, we have a very long time to wait inside wait() method. Ideally, the ConnectThread.run() will either make a connection or fail if socketTimeout and connectTimeout are working as expected. However, the second bug causes makeConnection in ConnectThread.run to be stuck forever, which means the calling thread has to wait until the long delay has elapsed.

To Reproduce

1. Inject a backward clock jump on the client. This can be a real backward clockjump, or you can manually set the delay to a very large value here.
2. Make enableSSL wait on the socket forever. This can be done by adding an iptables rule that drops all packets to the postgresql port.
   Please note that you need to drop all packets after the connection is established and before enableSSL is called. (one way is to add sleep before enableSSL) Otherwise, you will see Connection timeout error because a connection cannot be established.

Expected behaviour
The query fails after loginTimeout has elapsed.

[bokken]

Are there other timeouts (outside the Driver class) also using currentTimeMillis?
I am in favor of using nanoTime for these calculations[1], but want to be consistent through project.

[1] - https://stackoverflow.com/a/54566928/676877

[davecramer]

Just happened by this one

pgjdbc/pgjdbc/src/main/java/org/postgresql/core/v3/replication/V3PGReplicationStream.java

Line 59 in 381cf45

this.lastStatusUpdate = System.currentTimeMillis() - updateIntervalMs;

That said I think fixing that should be done in another PR

[stevenliang16]

@bokken @davecramer Thanks for the reply! I am also in favor of using nanoTime for calculating timeouts. Will someone from your team take on the task or should I try to send out a PR for this? Thanks!

[davecramer]

hmmm apparently we still have some stragglers besides tests that use currentTimeMillis

[davecramer]

fixed by #1617