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

feat: capture stack trace for session checkout is now optional #2350

Merged
merged 6 commits into from
Apr 7, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ If you are using Maven without BOM, add this to your dependencies:
If you are using Gradle 5.x or later, add this to your dependencies:

```Groovy
implementation platform('com.google.cloud:libraries-bom:26.10.0')
implementation platform('com.google.cloud:libraries-bom:26.11.0')

implementation 'com.google.cloud:google-cloud-spanner'
```
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,10 @@ final class LeakedSessionException extends RuntimeException {
private LeakedSessionException() {
super("Session was checked out from the pool at " + clock.instant());
}

private LeakedSessionException(String message) {
super(message);
}
}

private enum SessionState {
Expand Down Expand Up @@ -1131,7 +1135,9 @@ void clearLeakedException() {
}

private void markCheckedOut() {
this.leakedException = new LeakedSessionException();
if (options.isTrackStackTraceOfSessionCheckout()) {
this.leakedException = new LeakedSessionException();
}
}

@Override
Expand Down Expand Up @@ -2324,6 +2330,16 @@ ListenableFuture<Void> closeAsync(ClosedException closedException) {
} else {
logger.log(Level.WARNING, "Leaked session", session.leakedException);
}
} else {
String message =
"Leaked session. "
+ "Call SessionOptions.Builder#setTrackStackTraceOfSessionCheckout(true) to start "
+ "tracking the call stack trace of the thread that checked out the session.";
if (options.isFailOnSessionLeak()) {
throw new LeakedSessionException(message);
} else {
logger.log(Level.WARNING, message);
}
}
}
for (final PooledSession session : ImmutableList.copyOf(allSessions)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public class SessionPoolOptions {
private final Duration removeInactiveSessionAfter;
private final ActionOnSessionNotFound actionOnSessionNotFound;
private final ActionOnSessionLeak actionOnSessionLeak;
private final boolean trackStackTraceOfSessionCheckout;
private final long initialWaitForSessionTimeoutMillis;
private final boolean autoDetectDialect;
private final Duration waitForMinSessions;
Expand All @@ -65,6 +66,7 @@ private SessionPoolOptions(Builder builder) {
this.actionOnExhaustion = builder.actionOnExhaustion;
this.actionOnSessionNotFound = builder.actionOnSessionNotFound;
this.actionOnSessionLeak = builder.actionOnSessionLeak;
this.trackStackTraceOfSessionCheckout = builder.trackStackTraceOfSessionCheckout;
this.initialWaitForSessionTimeoutMillis = builder.initialWaitForSessionTimeoutMillis;
this.loopFrequency = builder.loopFrequency;
this.keepAliveIntervalMinutes = builder.keepAliveIntervalMinutes;
Expand All @@ -87,6 +89,8 @@ public boolean equals(Object o) {
&& Objects.equals(this.actionOnExhaustion, other.actionOnExhaustion)
&& Objects.equals(this.actionOnSessionNotFound, other.actionOnSessionNotFound)
&& Objects.equals(this.actionOnSessionLeak, other.actionOnSessionLeak)
&& Objects.equals(
this.trackStackTraceOfSessionCheckout, other.trackStackTraceOfSessionCheckout)
&& Objects.equals(
this.initialWaitForSessionTimeoutMillis, other.initialWaitForSessionTimeoutMillis)
&& Objects.equals(this.loopFrequency, other.loopFrequency)
Expand All @@ -107,6 +111,7 @@ public int hashCode() {
this.actionOnExhaustion,
this.actionOnSessionNotFound,
this.actionOnSessionLeak,
this.trackStackTraceOfSessionCheckout,
this.initialWaitForSessionTimeoutMillis,
this.loopFrequency,
this.keepAliveIntervalMinutes,
Expand Down Expand Up @@ -190,6 +195,10 @@ boolean isFailOnSessionLeak() {
return actionOnSessionLeak == ActionOnSessionLeak.FAIL;
}

public boolean isTrackStackTraceOfSessionCheckout() {
rajatbhatta marked this conversation as resolved.
Show resolved Hide resolved
return trackStackTraceOfSessionCheckout;
}

@VisibleForTesting
Duration getWaitForMinSessions() {
return waitForMinSessions;
Expand Down Expand Up @@ -234,6 +243,13 @@ public static class Builder {
private long initialWaitForSessionTimeoutMillis = 30_000L;
private ActionOnSessionNotFound actionOnSessionNotFound = ActionOnSessionNotFound.RETRY;
private ActionOnSessionLeak actionOnSessionLeak = ActionOnSessionLeak.WARN;
/**
* Capture the call stack of the thread that checked out a session of the pool. Can be disabled
* by customers who do not want this, for example if their monitoring systems think this is an
* exception that they need to log.
rajatbhatta marked this conversation as resolved.
Show resolved Hide resolved
*/
private boolean trackStackTraceOfSessionCheckout = true;

private long loopFrequency = 10 * 1000L;
private int keepAliveIntervalMinutes = 30;
private Duration removeInactiveSessionAfter = Duration.ofMinutes(55L);
Expand All @@ -253,6 +269,7 @@ private Builder(SessionPoolOptions options) {
this.initialWaitForSessionTimeoutMillis = options.initialWaitForSessionTimeoutMillis;
this.actionOnSessionNotFound = options.actionOnSessionNotFound;
this.actionOnSessionLeak = options.actionOnSessionLeak;
this.trackStackTraceOfSessionCheckout = options.trackStackTraceOfSessionCheckout;
this.loopFrequency = options.loopFrequency;
this.keepAliveIntervalMinutes = options.keepAliveIntervalMinutes;
this.removeInactiveSessionAfter = options.removeInactiveSessionAfter;
Expand Down Expand Up @@ -393,6 +410,20 @@ Builder setFailOnSessionLeak() {
return this;
}

/**
* Sets whether the session pool should capture the call stack trace when a session is checked
* out of the pool. This will internally prepare a {@link
* com.google.cloud.spanner.SessionPool.LeakedSessionException} that will only be thrown if the
* session is actually leaked. This makes it easier to debug session leaks, as the stack trace
* of the thread that checked out the session will be available in the exception.
*
* <p>Some monitoring tools might log these exceptions even
rajatbhatta marked this conversation as resolved.
Show resolved Hide resolved
*/
public Builder setTrackStackTraceOfSessionCheckout(boolean trackStackTraceOfSessionCheckout) {
this.trackStackTraceOfSessionCheckout = trackStackTraceOfSessionCheckout;
return this;
}

/**
* @deprecated This configuration value is no longer in use. The session pool does not prepare
* any sessions for read/write transactions. Instead, a transaction will automatically be
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@
import com.google.api.gax.grpc.testing.LocalChannelProvider;
import com.google.cloud.NoCredentials;
import com.google.cloud.spanner.MockSpannerServiceImpl.SimulatedExecutionTime;
import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult;
import com.google.cloud.spanner.SessionPool.LeakedSessionException;
import com.google.protobuf.ListValue;
import com.google.protobuf.Value;
import com.google.spanner.v1.ResultSetMetadata;
import com.google.spanner.v1.StructType;
import com.google.spanner.v1.StructType.Field;
import com.google.spanner.v1.Type;
import com.google.spanner.v1.TypeCode;
import io.grpc.Server;
import io.grpc.StatusRuntimeException;
import io.grpc.inprocess.InProcessServerBuilder;
Expand Down Expand Up @@ -94,6 +103,66 @@ public void tearDown() {
spanner.close();
}

@Test
public void testIgnoreLeakedSession() {
for (boolean trackStackTraceofSessionCheckout : new boolean[] {true, false}) {
SpannerOptions.Builder builder =
SpannerOptions.newBuilder()
.setProjectId("[PROJECT]")
.setChannelProvider(channelProvider)
.setCredentials(NoCredentials.getInstance());
builder.setSessionPoolOption(
SessionPoolOptions.newBuilder()
.setMinSessions(0)
.setMaxSessions(2)
.setIncStep(1)
.setFailOnSessionLeak()
.setTrackStackTraceOfSessionCheckout(trackStackTraceofSessionCheckout)
.build());
Spanner spanner = builder.build().getService();
DatabaseClient client =
spanner.getDatabaseClient(DatabaseId.of("[PROJECT]", "[INSTANCE]", "[DATABASE]"));
mockSpanner.putStatementResult(
StatementResult.query(
Statement.of("SELECT 1"),
com.google.spanner.v1.ResultSet.newBuilder()
.setMetadata(
ResultSetMetadata.newBuilder()
.setRowType(
StructType.newBuilder()
.addFields(
Field.newBuilder()
.setName("c")
.setType(
Type.newBuilder().setCode(TypeCode.INT64).build())
.build())
.build())
.build())
.addRows(
ListValue.newBuilder()
.addValues(Value.newBuilder().setStringValue("1").build())
.build())
.build()));

// Start a read-only transaction without closing it before closing the Spanner instance.
// This will cause a session leak.
ReadOnlyTransaction transaction = client.readOnlyTransaction();
try (ResultSet resultSet = transaction.executeQuery(Statement.of("SELECT 1"))) {
while (resultSet.next()) {
// ignore
}
}
LeakedSessionException exception = assertThrows(LeakedSessionException.class, spanner::close);
// The top of the stack trace will be "markCheckedOut" if we keep track of the point where the
// session was checked out, while it will be "closeAsync" if we don't. In the latter case, we
// get the stack trace of the method that tries to close the Spanner instance, while in the
// former the stack trace will contain the method that checked out the session.
assertEquals(
trackStackTraceofSessionCheckout ? "markCheckedOut" : "closeAsync",
exception.getStackTrace()[0].getMethodName());
}
}

@Test
public void testReadWriteTransactionExceptionOnCreateSession() {
readWriteTransactionTest(
Expand Down