Skip to content

Fix session perf. in synchronize/http req. context #66

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

Open
lalmeras wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix session perf. in synchronize/http req. context #66

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 29 additions & 4 deletions ...act/notifier/web/application/administration/page/AdministrationArtifactPortfolioPage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package fr.openwide.maven.artifact.notifier.web.application.administration.page;

import java.util.concurrent.atomic.AtomicReference;

import org.apache.wicket.markup.html.WebPage;
import org.apache.wicket.markup.html.link.Link;
import org.apache.wicket.model.IModel;
Expand All @@ -10,6 +12,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import fr.openwide.core.jpa.exception.SecurityServiceException;
import fr.openwide.core.jpa.exception.ServiceException;
import fr.openwide.core.wicket.more.link.descriptor.IPageLinkDescriptor;
import fr.openwide.core.wicket.more.link.descriptor.builder.LinkDescriptorBuilder;
import fr.openwide.core.wicket.more.markup.html.template.model.BreadCrumbElement;
Expand Down Expand Up @@ -51,12 +55,33 @@ public AdministrationArtifactPortfolioPage(PageParameters parameters) {

@Override
public void onClick() {
final AtomicReference<Exception> exception = new AtomicReference<Exception>(null);
try {
mavenSynchronizationService.synchronizeAllArtifactsAndNotifyUsers();
getSession().success(getString("administration.artifact.syncAll.success"));
} catch (Exception e) {
LOGGER.error("Error occured while synchronizing artifacts", e);
Thread thread = new Thread(new Runnable() {
@Override
public void run() {
try {
mavenSynchronizationService.synchronizeAllArtifactsAndNotifyUsers();
} catch (ServiceException|SecurityServiceException|InterruptedException e) {
exception.set(e);
if (e instanceof InterruptedException) {
Thread.currentThread().interrupt();
}
}
}
});
thread.start();
thread.join();
Copy link

@martin-g martin-g Oct 5, 2018

Choose a reason for hiding this comment

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

Why do you start it in a new Thread if you need to wait for it here ? Why not just execute it in the current thread ?

Copy link
Member Author

@lalmeras lalmeras Oct 5, 2018

Choose a reason for hiding this comment

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

Synchronization process need to run in a separate Hibernate session for performance reason. Running it into a separate Thread and joining the thread is a way to run it with an isolated session without interrupting the HTTP thread&request-scoped session.

Another way to proceed would be to deal with Thread-scoped/TransactionSynchronizationManager registered session (save, replace, artifact synchronization, restore): it does not seem to me there is an helper for this ?

Copy link

@martin-g martin-g Oct 5, 2018

Choose a reason for hiding this comment

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

You know better the details how this leads to better performance in your app, but this is how I see it from outside:

Wicket is thread-per-request based so if you use OpenSessionInViewFilter then you have one request scoped DB transaction. If you need to start a new one for the Maven sync then you can use @Propagation.RequiresNew for this service method. If you don't make it in a separate transaction then the only drawback I see is that the request-scoped transaction may get rolled back if something fails later.

Copy link
Member Author

@lalmeras lalmeras Oct 5, 2018

Choose a reason for hiding this comment

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

We are transaction-less at the time I do this trick (only session is opened at request scope, transaction are kept at service scope). This pattern is a bit original and can be tricky when rollbacks are involved, but work flawlessly for a lot of our projects.

I do not use @transactional mechanism as I do not want to wrap whole synchronization in a transaction (we deal with transaction around each artifact synchronization). I do not dig into @transactional configuration to see if it is possible to let it open a session without starting a new transaction.

As transactions are also separated (Thread trick also isolates transactions), there is no request-scoped transaction, and page / synchronization data do not need to be synced, I do not think there is any problem with transaction isolation / rollback.

} catch (InterruptedException|RuntimeException e) {
exception.set(e);
if (e instanceof InterruptedException) {
Thread.currentThread().interrupt();
}
}
if (exception.get() != null) {
getSession().error(getString("administration.artifact.syncAll.error"));
} else {
getSession().success(getString("administration.artifact.syncAll.success"));
}
}
});
Expand Down