Skip to content

Fix bug where topology query could execute indefinitely #416

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.

Sign up for GitHub

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

Merged
sergiyvamz merged 8 commits into from
May 11, 2023
Merged

Fix bug where topology query could execute indefinitely #416

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2f86c2b
Fix bug where topology query could execute indefinitely
aaron-congo Apr 28, 2023
3f24ad4
Fix qodana warning
aaron-congo Apr 28, 2023
c7639e0
Wait for setNetworkTimeout to complete
aaron-congo May 4, 2023
91401c3
Fix checkstyle
aaron-congo May 4, 2023
acf6d28
PR suggestions
aaron-congo May 4, 2023
d3bc28c
Uncomment required code
aaron-congo May 4, 2023
daac7d4
Remove unnecessary imports
aaron-congo May 4, 2023
ef33e4b
Merge remote-tracking branch 'upstream/main' into topology-query-timeout
aaron-congo May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions wrapper/src/main/java/software/amazon/jdbc/hostlistprovider/AuroraHostListProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import java.util.Properties;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Logger;
Expand Down Expand Up @@ -75,6 +77,7 @@ public class AuroraHostListProvider implements DynamicHostListProvider {
+ "This pattern is required to be specified for IP address or custom domain connections to AWS RDS "
+ "clusters. Otherwise, if unspecified, the pattern will be automatically created for AWS RDS clusters.");

private static final Executor NETWORK_TIMEOUT_EXECUTOR = Executors.newSingleThreadExecutor();
private final HostListProviderService hostListProviderService;
private final String originalUrl;
private RdsUrlType rdsUrlType;
Expand All @@ -93,6 +96,7 @@ public class AuroraHostListProvider implements DynamicHostListProvider {
public static final CacheMap<String, String> suggestedPrimaryClusterIdCache = new CacheMap<>();
public static final CacheMap<String, Boolean> primaryClusterIdCache = new CacheMap<>();

private static final int defaultTopologyQueryTimeoutMs = 5000;
private final ReentrantLock lock = new ReentrantLock();
protected String clusterId;
protected HostSpec clusterInstanceTemplate;
Expand Down Expand Up @@ -333,11 +337,27 @@ protected List<HostSpec> queryForTopology(final Connection conn) throws SQLExcep
this.topologyAwareDialect = (TopologyAwareDatabaseCluster) this.hostListProviderService.getDialect();
}

int networkTimeout = -1;
try {
networkTimeout = conn.getNetworkTimeout();
// The topology query is not monitored by the EFM plugin, so it needs a socket timeout
if (networkTimeout == 0) {
Copy link
Contributor

@congoamz congoamz Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we override the user's socket timeout here even if it is set to a non-zero value?

Copy link
Contributor

@karenc-bq karenc-bq Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation for that?

Copy link
Contributor

@sergiyvamz sergiyvamz Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should apply default 5000ms socket timeout just in case the current one is set to indefinite wait (it doesn't matter if this indefinite was a user explicit setting or just lack of setting)

Copy link
Contributor

@congoamz congoamz Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they've set their socket timeout to accommodate their longest running queries it could be awhile before a problem is detected with the topology query. The topology query itself should be pretty quick to return. However, I lean towards keeping the code as is, I'd prefer to abide by the settings they gave us as much as possible. Just thought I'd bring it up to see if you agree

Copy link
Contributor

@congoamz congoamz Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sergiyvamz won't the value returned by getNetworkTimeout be 0 regardless of if the user explicitly set it to zero or just didn't set it at all?

Copy link
Contributor

@sergiyvamz sergiyvamz May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it is. But it makes sense to verify.

Copy link
Contributor

@congoamz congoamz May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked and the default value is 0 with all drivers

conn.setNetworkTimeout(NETWORK_TIMEOUT_EXECUTOR, defaultTopologyQueryTimeoutMs);
Copy link
Contributor

@sergiyvamz sergiyvamz Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it synchronous? We shouldn't need to check executor to check it timeout is applied?

}
} catch (SQLException e) {
LOGGER.warning(() -> Messages.get("AuroraHostListProvider.errorGettingNetworkTimeout",
new Object[] {e.getMessage()}));
}

try (final Statement stmt = conn.createStatement();
final ResultSet resultSet = stmt.executeQuery(this.topologyAwareDialect.getTopologyQuery())) {
return processQueryResults(resultSet);
} catch (final SQLSyntaxErrorException e) {
throw new SQLException(Messages.get("AuroraHostListProvider.invalidQuery"), e);
} finally {
if (networkTimeout == 0 && !conn.isClosed()) {
conn.setNetworkTimeout(NETWORK_TIMEOUT_EXECUTOR, networkTimeout);
}
}
}

Expand Down
1 change: 1 addition & 0 deletions wrapper/src/main/resources/messages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ AuroraHostListProvider.invalidQuery=Error obtaining host list. Provided database
AuroraHostListProvider.invalidDialect=Expecting a dialect that supports a cluster topology.
AuroraHostListProvider.invalidDialectForGetHostRole=An Aurora dialect is required to analyze a host's role. The detected dialect was ''{0}''
AuroraHostListProvider.errorGettingHostRole=An error occurred while obtaining the connected host's role. This could occur if the connection is broken or if you are not connected to an Aurora database.
AuroraHostListProvider.errorGettingNetworkTimeout=An error occurred while getting the connection network timeout: {0}

# AWS Credentials Manager
AwsCredentialsManager.nullProvider=The configured AwsCredentialsProvider was null. If you have configured the AwsCredentialsManager to use a custom AwsCredentialsProviderHandler, please ensure the handler does not return null.
Expand Down