Skip to content

Commit

Permalink
Fix connection leak in modifyObjectDynamically
Browse files Browse the repository at this point in the history
Also:

1. DB connections in HikariCP are now stored with autocommit = false
(avoiding unnecessary resetting of this flag each time a connection
is returned to the pool).

2. Updated HikariCP to 3.4.5 (the latest 3.x version).
  • Loading branch information
mederly committed Apr 15, 2021
1 parent 49da8d7 commit 303168e
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 34 deletions.
2 changes: 1 addition & 1 deletion pom.xml
Expand Up @@ -385,7 +385,7 @@
<dependency>
<groupId>com.zaxxer</groupId>
<artifactId>HikariCP</artifactId>
<version>3.3.1</version>
<version>3.4.5</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
Expand Down
Expand Up @@ -570,43 +570,44 @@ public <T extends ObjectType> ModifyObjectResult<T> modifyObjectDynamicallyAttem

LOGGER_PERFORMANCE.debug("> modify object dynamically {}, oid={}", type.getSimpleName(), oid);

Session session = baseHelper.beginTransaction();
try (Session session = baseHelper.beginTransaction()) {

PrismObject<T> objectBefore;
Collection<? extends ItemDelta<?, ?>> modifications;
try {
objectBefore = objectRetriever.getObjectInternal(session, type, oid, getOptions, true);
LOGGER.trace("Object retrieved:\n{}", objectBefore.debugDumpLazily(1));
PrismObject<T> objectBefore;
Collection<? extends ItemDelta<?, ?>> modifications;
try {
objectBefore = objectRetriever.getObjectInternal(session, type, oid, getOptions, true);
LOGGER.trace("Object retrieved:\n{}", objectBefore.debugDumpLazily(1));

// Intentionally within this try-catch block because this call must be covered by proper exception handling.
modifications = modificationsSupplier.get(objectBefore.asObjectable());
LOGGER.trace("Modifications computed:\n{}", DebugUtil.debugDumpLazily(modifications, 1));
} catch (ObjectNotFoundException ex) {
GetOperationOptions rootOptions = SelectorOptions.findRootOptions(getOptions);
baseHelper.rollbackTransaction(session, ex, result, !GetOperationOptions.isAllowNotFound(rootOptions));
throw ex;
} catch (SchemaException ex) {
baseHelper.rollbackTransaction(session, ex, "Schema error while getting object with oid: "
+ oid + ". Reason: " + ex.getMessage(), result, true);
throw ex;
} catch (DtoTranslationException | RuntimeException ex) {
baseHelper.handleGeneralException(ex, session, result);
throw new AssertionError("shouldn't be here");
}
// Intentionally within this try-catch block because this call must be covered by proper exception handling.
modifications = modificationsSupplier.get(objectBefore.asObjectable());
LOGGER.trace("Modifications computed:\n{}", DebugUtil.debugDumpLazily(modifications, 1));
} catch (ObjectNotFoundException ex) {
GetOperationOptions rootOptions = SelectorOptions.findRootOptions(getOptions);
baseHelper.rollbackTransaction(session, ex, result, !GetOperationOptions.isAllowNotFound(rootOptions));
throw ex;
} catch (SchemaException ex) {
baseHelper.rollbackTransaction(session, ex, "Schema error while getting object with oid: "
+ oid + ". Reason: " + ex.getMessage(), result, true);
throw ex;
} catch (DtoTranslationException | RuntimeException ex) {
baseHelper.handleGeneralException(ex, session, result);
throw new AssertionError("shouldn't be here");
}

if (modifications.isEmpty() && !RepoModifyOptions.isForceReindex(modifyOptions)) {
LOGGER.debug("Modification list is empty, nothing was modified.");
session.getTransaction().commit();
result.recordStatus(OperationResultStatus.SUCCESS, "Computed modification list is empty");
return new ModifyObjectResult<>(objectBefore, objectBefore, modifications);
}
if (modifications.isEmpty() && !RepoModifyOptions.isForceReindex(modifyOptions)) {
LOGGER.debug("Modification list is empty, nothing was modified.");
session.getTransaction().commit();
result.recordStatus(OperationResultStatus.SUCCESS, "Computed modification list is empty");
return new ModifyObjectResult<>(objectBefore, objectBefore, modifications);
}

try {
// TODO: eliminate redundant getObjectInternal call in modifyObjectAttempt
return modifyObjectAttempt(type, oid, modifications, null, modifyOptions, attempt, result, sqlRepositoryService,
noFetchExtensionValueInsertionForbidden, session);
} catch (PreconditionViolationException e) {
throw new SystemException("Unexpected PreconditionViolationException: " + e.getMessage(), e);
try {
// TODO: eliminate redundant getObjectInternal call in modifyObjectAttempt
return modifyObjectAttempt(type, oid, modifications, null, modifyOptions, attempt, result, sqlRepositoryService,
noFetchExtensionValueInsertionForbidden, session);
} catch (PreconditionViolationException e) {
throw new SystemException("Unexpected PreconditionViolationException: " + e.getMessage(), e);
}
}
}

Expand Down
Expand Up @@ -111,6 +111,7 @@ private HikariConfig createHikariConfig() {
}

config.setInitializationFailTimeout(configuration.getInitializationFailTimeout());
config.setAutoCommit(false);

return config;
}
Expand Down

0 comments on commit 303168e

Please sign in to comment.