Skip to content

SLING-12394 improve ResourceResolver handling #39

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
joerghoh wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SLING-12394 improve ResourceResolver handling #39

Show file tree
Hide file tree
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
21 changes: 5 additions & 16 deletions src/main/java/org/apache/sling/event/impl/jobs/JobManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,7 @@ private boolean internalRemoveJobById(final String jobId, final boolean forceRem
final boolean isHistoryJob = this.configuration.isStoragePath(job.getResourcePath());
// if history job, simply remove - otherwise move to history!
if ( isHistoryJob ) {
final ResourceResolver resolver = this.configuration.createResourceResolver();
try {
try (final ResourceResolver resolver = this.configuration.createResourceResolver();) {
final Resource jobResource = resolver.getResource(job.getResourcePath());
if ( jobResource != null ) {
resolver.delete(jobResource);
Expand All @@ -280,8 +279,6 @@ private boolean internalRemoveJobById(final String jobId, final boolean forceRem
} catch ( final PersistenceException pe) {
logger.warn("Unable to remove job at " + job.getResourcePath(), pe);
result = false;
} finally {
resolver.close();
}
} else {
final JobHandler jh = new JobHandler(job, null, this.configuration);
Expand Down Expand Up @@ -309,9 +306,8 @@ public Job addJob(String topic, Map<String, Object> properties) {
@Override
public Job getJobById(final String id) {
logger.debug("Getting job by id: {}", id);
final ResourceResolver resolver = this.configuration.createResourceResolver();
final StringBuilder buf = new StringBuilder(64);
try {
try (final ResourceResolver resolver = this.configuration.createResourceResolver();) {

buf.append("/jcr:root");
buf.append(this.configuration.getJobsBasePathWithSlash());
Expand Down Expand Up @@ -342,8 +338,6 @@ public Job getJobById(final String id) {
}
} catch (final QuerySyntaxException qse) {
logger.warn("Query syntax wrong " + buf.toString(), qse);
} finally {
resolver.close();
}
logger.debug("Job not found with id: {}", id);
return null;
Expand Down Expand Up @@ -400,9 +394,8 @@ public Collection<Job> findJobs(final QueryType type,
|| type == QueryType.GIVEN_UP
|| type == QueryType.STOPPED;
final List<Job> result = new ArrayList<>();
final ResourceResolver resolver = this.configuration.createResourceResolver();
final StringBuilder buf = new StringBuilder(64);
try {
try (final ResourceResolver resolver = this.configuration.createResourceResolver();) {
buf.append(buildBaseQuery(this.configuration.getJobsBasePathWithSlash(), topic, type, isHistoryQuery));

if ( templates != null && templates.length > 0 ) {
Expand Down Expand Up @@ -502,8 +495,6 @@ public Collection<Job> findJobs(final QueryType type,
}
} catch (final QuerySyntaxException qse) {
logger.warn("Query syntax wrong " + buf.toString(), qse);
} finally {
resolver.close();
}
return result;
}
Expand Down Expand Up @@ -587,8 +578,8 @@ private Job addJobInternal(final String jobTopic,
logger.debug("Persisting job {} into queue {}, target={}",
Utility.toString(jobTopic, jobProperties), info.queueName, info.targetId);
}
final ResourceResolver resolver = this.configuration.createResourceResolver();
try {

try (final ResourceResolver resolver = this.configuration.createResourceResolver();) {
final JobImpl job = this.writeJob(resolver,
jobTopic,
jobProperties,
Expand All @@ -604,8 +595,6 @@ private Job addJobInternal(final String jobTopic,
} catch (final PersistenceException re ) {
// something went wrong, so let's log it
this.logger.error("Exception during persisting new job '" + Utility.toString(jobTopic, jobProperties) + "'", re);
} finally {
resolver.close();
}
if ( errors != null ) {
errors.add("Unable to persist new job.");
Expand Down
16 changes: 7 additions & 9 deletions src/main/java/org/apache/sling/event/impl/jobs/config/JobManagerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.apache.sling.event.impl.support.ResourceHelper;
import org.apache.sling.event.jobs.Job;
import org.apache.sling.serviceusermapping.ServiceUserMapped;
import org.jetbrains.annotations.NotNull;
import org.osgi.framework.Constants;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
Expand Down Expand Up @@ -231,15 +232,12 @@ protected void activate(final Map<String, Object> props, final Config config) {
this.historyCleanUpRemovedJobs = config.cleanup_period();

// create initial resources
final ResourceResolver resolver = this.createResourceResolver();
try {
try (final ResourceResolver resolver = this.createResourceResolver();) {
ResourceHelper.getOrCreateBasePath(resolver, this.getLocalJobsPath());
ResourceHelper.getOrCreateBasePath(resolver, this.getUnassignedJobsPath());
} catch ( final PersistenceException pe ) {
logger.error("Unable to create default paths: " + pe.getMessage(), pe);
throw new RuntimeException(pe);
} finally {
resolver.close();
}
this.active.set(true);

Expand Down Expand Up @@ -308,21 +306,21 @@ public boolean isActive() {
* This ResourceResolver provides read and write access to all resources relevant for the event
* and job handling.
*
* @return A resource resolver or {@code null} if the component is already deactivated.
* @return A resource resolver
* @throws RuntimeException if the resolver can't be created.
*/
public ResourceResolver createResourceResolver() {
ResourceResolver resolver = null;
public @NotNull ResourceResolver createResourceResolver() {
Copy link
Copy Markdown
Contributor

@stefan-egli stefan-egli Oct 3, 2024

Choose a reason for hiding this comment

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

IIUC this change will result in exceptions rather than silent swallows if the factory is null. That might its own consequences. What is the reasoning behind this change?

Copy link
Copy Markdown

@jsedding jsedding Oct 4, 2024

Choose a reason for hiding this comment

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

I agree with @stefan-egli's concern. There are quite a few places where we had a null check before.

@joerghoh I suggest to throw a NullPointerException (or something more specific) instead of a RuntimeException and to catch and log this exception in the places where we used to have a null check.

Copy link
Copy Markdown
Contributor Author

@joerghoh joerghoh Jun 23, 2025

Choose a reason for hiding this comment

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

These null-checks seem to be added rather by accident than really planned. Otherwise all places where no null-checks exist would need to be augmented with these checks as well, and that was not a problem for a very long time.

Silently swallowing these exceptions (and therefor not executing the expected action) is also not a good pattern. On the other hand side the LoginException happens mostly due to configuration issues (also rare, because createResourceResolver code is not exported, and thus will only be used by the Sling event code), and therefor they should be easily fixable. So in the end the actual type of issue does not seem to be a problem, which is likely to happen frequently.
And for that reason I think it's possible to adjust the way errors are handled, as this is contained in this code. We might want to add a Thread.UncaughtExceptionHandler to the threads of the job threadpool(s).

final ResourceResolverFactory factory = this.resourceResolverFactory;
if ( factory != null ) {
try {
resolver = this.resourceResolverFactory.getServiceResourceResolver(null);
return this.resourceResolverFactory.getServiceResourceResolver(null);
} catch ( final LoginException le) {
logger.error("Unable to create new resource resolver: " + le.getMessage(), le);
throw new RuntimeException(le);
}
} else {
throw new RuntimeException ("ResourceResolverFactory is null, cannot create ResourceResolver");
}
return resolver;
}

/**
Expand Down
7 changes: 2 additions & 5 deletions src/main/java/org/apache/sling/event/impl/jobs/queues/JobQueueImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,8 +641,7 @@ public synchronized void removeAll() {

if ( !topics.isEmpty() ) {

final ResourceResolver resolver = this.services.configuration.createResourceResolver();
try {
try (final ResourceResolver resolver = this.services.configuration.createResourceResolver();) {
final Resource baseResource = resolver.getResource(this.services.configuration.getLocalJobsPath());

// sanity check - should never be null
Expand Down Expand Up @@ -674,12 +673,10 @@ public boolean handle(final JobImpl job) {
}
try {
resolver.commit();
} catch ( final PersistenceException ignore) {
} catch (final PersistenceException ignore) {
logger.error("Unable to remove jobs", ignore);
}
}
} finally {
resolver.close();
}
}
}
Expand Down
5 changes: 1 addition & 4 deletions src/main/java/org/apache/sling/event/impl/jobs/queues/QueueJobCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,7 @@ private void loadJobs( final String queueName, final Set<String> checkingTopics,

final Map<String, List<JobImpl>> topicCache = new HashMap<String, List<JobImpl>>();

final ResourceResolver resolver = this.configuration.createResourceResolver();
try {
try (final ResourceResolver resolver = this.configuration.createResourceResolver();) {
final Resource baseResource = resolver.getResource(this.configuration.getLocalJobsPath());
// sanity check - should never be null
if ( baseResource != null ) {
Expand All @@ -234,8 +233,6 @@ private void loadJobs( final String queueName, final Set<String> checkingTopics,
}
}
}
} finally {
resolver.close();
}
orderTopics(topicCache);

Expand Down
5 changes: 1 addition & 4 deletions src/main/java/org/apache/sling/event/impl/jobs/queues/QueueManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,8 +426,7 @@ void fullTopicScan() {
private Set<String> scanTopics() {
final Set<String> topics = new HashSet<>();

final ResourceResolver resolver = this.configuration.createResourceResolver();
try {
try (final ResourceResolver resolver = this.configuration.createResourceResolver();) {
final Resource baseResource = resolver.getResource(this.configuration.getLocalJobsPath());

// sanity check - should never be null
Expand All @@ -440,8 +439,6 @@ private Set<String> scanTopics() {
topics.add(topic);
}
}
} finally {
resolver.close();
}
return topics;
}
Expand Down
5 changes: 1 addition & 4 deletions src/main/java/org/apache/sling/event/impl/jobs/scheduling/JobSchedulerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,7 @@ public int getTotalNumberOfScheduledJobs() {
* @param flag The corresponding flag
*/
public void setSuspended(final ScheduledJobInfoImpl info, final boolean flag) {
final ResourceResolver resolver = configuration.createResourceResolver();
try {
try (final ResourceResolver resolver = configuration.createResourceResolver();) {
final StringBuilder sb = new StringBuilder(this.configuration.getScheduledJobsPath(true));
sb.append(ResourceHelper.filterName(info.getName()));
final String path = sb.toString();
Expand All @@ -496,8 +495,6 @@ public void setSuspended(final ScheduledJobInfoImpl info, final boolean flag) {
} catch (final PersistenceException pe) {
// we ignore the exception if removing fails
ignoreException(pe);
} finally {
resolver.close();
}
}

Expand Down
99 changes: 38 additions & 61 deletions src/main/java/org/apache/sling/event/impl/jobs/scheduling/ScheduledJobHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,36 +141,31 @@ public void run() {
}

private void scan() {
final ResourceResolver resolver = configuration.createResourceResolver();
if ( resolver != null ) {
Comment on lines -144 to -145
Copy link
Copy Markdown

@jsedding jsedding Oct 4, 2024

Choose a reason for hiding this comment

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

Here we have a null check. Should we handle the exception here instead?

try {
logger.debug("Scanning for scheduled jobs...");
final String path = this.configuration.getScheduledJobsPath(false);
final Resource startResource = resolver.getResource(path);
if ( startResource != null ) {
final Map<String, Holder> newScheduledJobs = new HashMap<String, Holder>();
synchronized ( this.scheduledJobs ) {
for(final Resource rsrc : startResource.getChildren()) {
if ( !isRunning.get() ) {
break;
}
handleAddOrUpdate(newScheduledJobs, rsrc);
try (final ResourceResolver resolver = configuration.createResourceResolver();) {
logger.debug("Scanning for scheduled jobs...");
final String path = this.configuration.getScheduledJobsPath(false);
final Resource startResource = resolver.getResource(path);
if ( startResource != null ) {
final Map<String, Holder> newScheduledJobs = new HashMap<String, Holder>();
synchronized ( this.scheduledJobs ) {
for(final Resource rsrc : startResource.getChildren()) {
if ( !isRunning.get() ) {
break;
}
if ( isRunning.get() ) {
for(final Holder h : this.scheduledJobs.values()) {
if ( h.info != null ) {
this.jobScheduler.unscheduleJob(h.info);
}
handleAddOrUpdate(newScheduledJobs, rsrc);
}
if ( isRunning.get() ) {
for(final Holder h : this.scheduledJobs.values()) {
if ( h.info != null ) {
this.jobScheduler.unscheduleJob(h.info);
}
this.scheduledJobs.clear();
this.scheduledJobs.putAll(newScheduledJobs);
}
this.scheduledJobs.clear();
this.scheduledJobs.putAll(newScheduledJobs);
}
}
logger.debug("Finished scanning for scheduled jobs...");
} finally {
resolver.close();
}
logger.debug("Finished scanning for scheduled jobs...");
}
}

Expand Down Expand Up @@ -235,8 +230,7 @@ private Map<String, Object> writeScheduledJob(final String jobTopic,
final boolean suspend,
final List<ScheduleInfoImpl> scheduleInfos)
throws PersistenceException {
final ResourceResolver resolver = this.configuration.createResourceResolver();
try {
try (final ResourceResolver resolver = this.configuration.createResourceResolver();) {
// create properties
final Map<String, Object> properties = new HashMap<String, Object>();

Expand Down Expand Up @@ -286,8 +280,6 @@ private Map<String, Object> writeScheduledJob(final String jobTopic,
properties.put(ResourceHelper.PROPERTY_SCHEDULE_INFO, scheduleInfos);

return properties;
} finally {
resolver.close();
}
}

Expand Down Expand Up @@ -331,23 +323,18 @@ public void run() {
}
}
if ( !updateJobs.isEmpty() && isRunning.get() ) {
ResourceResolver resolver = configuration.createResourceResolver();
if ( resolver != null ) {
Comment on lines -334 to -335
Copy link
Copy Markdown

@jsedding jsedding Oct 4, 2024

Choose a reason for hiding this comment

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

Here we have a null check. Should we handle the exception here instead?

try {
for(final Map.Entry<String, Holder> entry : updateJobs.entrySet()) {
final String path = configuration.getScheduledJobsPath(true) + entry.getKey();
final Resource rsrc = resolver.getResource(path);
if ( !isRunning.get() ) {
break;
}
if ( rsrc != null ) {
synchronized ( scheduledJobs ) {
handleAddOrUpdate(scheduledJobs, rsrc);
}
try (ResourceResolver resolver = configuration.createResourceResolver();) {
for(final Map.Entry<String, Holder> entry : updateJobs.entrySet()) {
final String path = configuration.getScheduledJobsPath(true) + entry.getKey();
final Resource rsrc = resolver.getResource(path);
if ( !isRunning.get() ) {
break;
}
if ( rsrc != null ) {
synchronized ( scheduledJobs ) {
handleAddOrUpdate(scheduledJobs, rsrc);
}
}
} finally {
resolver.close();
}
}
}
Expand Down Expand Up @@ -387,17 +374,12 @@ public void handleAddUpdate(final String path) {
@Override
public void run() {
if ( isRunning.get() ) {
final ResourceResolver resolver = configuration.createResourceResolver();
if ( resolver != null ) {
Comment on lines -390 to -391
Copy link
Copy Markdown

@jsedding jsedding Oct 4, 2024

Choose a reason for hiding this comment

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

Here we have a null check. Should we handle the exception here instead?

try {
final Resource rsrc = resolver.getResource(path);
if ( rsrc != null ) {
synchronized ( scheduledJobs ) {
handleAddOrUpdate(scheduledJobs, rsrc);
}
try (final ResourceResolver resolver = configuration.createResourceResolver();) {
final Resource rsrc = resolver.getResource(path);
if ( rsrc != null ) {
synchronized ( scheduledJobs ) {
handleAddOrUpdate(scheduledJobs, rsrc);
}
} finally {
resolver.close();
}
}
}
Expand Down Expand Up @@ -462,8 +444,8 @@ private void handleAddOrUpdate(final Map<String, Holder> newScheduledJobs, final
public void remove(final ScheduledJobInfoImpl info) {
final String scheduleKey = ResourceHelper.filterName(info.getName());

final ResourceResolver resolver = configuration.createResourceResolver();
try {

try (final ResourceResolver resolver = configuration.createResourceResolver();) {
final StringBuilder sb = new StringBuilder(configuration.getScheduledJobsPath(true));
sb.append(scheduleKey);
final String path = sb.toString();
Expand All @@ -476,8 +458,6 @@ public void remove(final ScheduledJobInfoImpl info) {
} catch (final PersistenceException pe) {
// we ignore the exception if removing fails
ignoreException(pe);
} finally {
resolver.close();
}

synchronized ( this.scheduledJobs ) {
Expand All @@ -490,8 +470,7 @@ public void remove(final ScheduledJobInfoImpl info) {

public void updateSchedule(final String scheduleName, final Collection<ScheduleInfo> scheduleInfo) {

final ResourceResolver resolver = configuration.createResourceResolver();
try {
try (final ResourceResolver resolver = configuration.createResourceResolver();) {
final String scheduleKey = ResourceHelper.filterName(scheduleName);

final StringBuilder sb = new StringBuilder(configuration.getScheduledJobsPath(true));
Expand Down Expand Up @@ -527,8 +506,6 @@ public void updateSchedule(final String scheduleName, final Collection<ScheduleI
logger.warn("Unable to update scheduled job " + scheduleName, pe);
}
}
} finally {
resolver.close();
}
}

Expand Down
Loading