Need help with resourceResolver closed errors

[valcohen]

I'm adding a feature to some longstanding, complex integration code in AEM 6.5 and discovering existing problems with invalid resource resolvers. Adding the feature is making them worse. This is some of the oldest code in our system, and has gone through several rounds of refactoring and other updates.

I'm unclear about the best way to manage the resource resolver and associated objects, especially as control goes from a service to Sling models that use a resolver or session to traverse or search the JCR. I'm hoping someone can review my code and give me some advice (@Jörg_Hoh , possible to get your thoughts?) Here's the general overview, followed by VERY stripped-down versions of the classes involved:

• We store a hierarchy of inventory in the AEM JCR, and update it with messages from an upstream system. The updates can affect a number of objects in the JCR. We combine those changes into models for another external DB and send outbound updates.
• Messages are received by a servlet (not shown) and stored in nodes in the JCR. A separate job is scheduled to run periodically and process the messages:
• MyScheduledTask.run() reads the messages, updates the JCR, and calls a service to update the external DB.
• dbUploadService.updateIndexFor() does a QueryBuilder search the JCR nodes affected by the update. For each hit, the service passes the hit.resource to modelFactory.createModel() to hydrate the index model which in turn hydrates entity models.

There are two types of models: a mostly POJO "index" model and familiar Sling Model "entities" which read data from the JCR. The index models instantiate various entity models, pull various model properties into a map, and that map gets serialized to JSON and sent to the external DB. The index models don't declare a resolver -- I'm not clear on what they're using when they do path traversal. The entity models declare an injected @SlingObject ResourceResolver.

I'm getting "Resource resolver is already closed" from the scheduled task when calling getPath() or listChildren() on a resource.
I'm getting "Resource resolver is already closed" and "This session has been closed" exceptions from the entity models when they try to do path traversal via getParent(), or when the upload service is trying to execute a query.

Any help appreciated! 

/************** SCHEDULED TASK  *********************/

public class MyScheduledTask implements Runnable {

    @Reference private ResourceResolverFactory  resourceResolverFactory;

    @Reference private DbUploadService          dbUploadService;

    private ResourceResolver resourceResolver;

    private ResourceResolver getResourceResolver() {

        if (resourceResolver == null || !resourceResolver.isLive()) {

            try {

                // I'm getting the resolver from a factory, not the Sling request, 

                // because this job runs on a schedule rather than on a servlet request

                Map<String, Object> serviceUserParam = new HashMap<String, Object>();

                serviceUserParam.put(ResourceResolverFactory.SUBSERVICE, "dbServiceUser");

                resourceResolver = resourceResolverFactory.getServiceResourceResolver(serviceUserParam);

            } catch (Exception e) {

                logger.error("Unable to obtain resource resolver", e);

                return null;    // return here so we don't return a dead resolver

            }

        }

        return resourceResolver;

    }

    private Session session;

    private Session getSession() {

        if (this.session == null || !this.session.isLive()) {

            session = this.getResourceResolver().adaptTo(Session.class);

        }

        return session;

    }

    private PageManager pageManager;

    private PageManager getPageManager() {

        if (pageManager == null) {  // TODO: also check the resolver status?

            pageManager = this.getResourceResolver().adaptTo(PageManager.class);

        }

        return pageManager;

    }

    private ArrayList<String> activationList;

    @Override

    public void run() {

        try {

            Resource msgFolder = this.getResourceResolver().getResource("/path/to/message/nodes");

            this.activationList = new ArrayList<>();

            Iterator<Resource> tasks = msgFolder.listChildren();

            while (tasks.hasNext()) {

                Resource task = tasks.next();

                if (task.getValueMap().containsKey("message")) {

                    processItem(task.getValueMap().get("message", String.class));

                    this.getResourceResolver().delete(task);

                    this.getSession().save();

                }

            }

            Iterator<String> activationIter = activationList.iterator();

            while (activationIter.hasNext()) {

                Resource resourceToActivate = this.getResourceResolver().getResource(activationIter.next());

                // check activation status, call replicator if last status was Activate

            }

        } catch (Exception e) {

            logger.error(e);

        } finally { // I could replace this with try-with-resources

            if (this.resourceResolver != null && this.resourceResolver.isLive()) {

            this.resourceResolver.close();

            }

        }

    }

    private void processItem(String message) {

        String idToFind = getIdFrom(message);

        Page thePage = findPage(this.getPageManager().getPage("/content/base/page/path"), idToFind);

        // lots of processing to update the item with values from the message, save the session

        thePage.getContentResource().adaptTo(Node.class).setProperty("foo", "value from message.foo");

        this.getSession().save();

        activationList.add(thePage.getPath()); // so caller can try to replicate

        // call a service to hydrate Sling models, send them to an external database

        updateExternalDb(idToFind, thePage.getContentResource());

    }

    private Page findPage(Page parentPage, String entityId) {

        Iterator<Page> children = parentPage.listChildren(); // <--- crash here, resolver is already closed

        while (children.hasNext()) {

            Page currentPage = children.next();

            ValueMap pageProps = currentPage.getProperties(); 

            if (pageProps.containsKey("entityId") && pageProps.get("entityId", "").equals(entityId)) {

                return currentPage;

            }

        }

        return null;
    }

    private void updateExternalDb(String id, Resource contentResource) {

        String changeRoot = contentResource.getParent().getPath();

        dbUploadService.updateIndexFor(id, "buildingId", changeRoot, Constants.MIXIN_BUILDING));

        dbUploadService.updateIndexFor(id, "buildingId", changeRoot, Constants.MIXIN_SUITE);

    }

}

/************************** DB UPLOAD SERVICE ********************************/

@Service

@Component

public class DbUploadServiceImpl implements DbUploadService {

    @Reference private ResourceResolverFactory resourceResolverFactory;

    @Reference private ModelFactory modelFactory;

    private ResourceResolver resourceResolver;

    private ResourceResolver getResourceResolver() {

        if (resourceResolver == null || !resourceResolver.isLive()) {

            try {

                Map<String, Object> serviceUserParam = new HashMap<String, Object>();

                serviceUserParam.put(ResourceResolverFactory.SUBSERVICE, "dbServiceUser");

                this.resourceResolver = resourceResolverFactory.getServiceResourceResolver(serviceUserParam);

            } catch (Exception e) {

                LOG.error("Unable to obtain resource resolver");

                return null;    // return here so we don't return a dead resolver

            }

        }

        return this.resourceResolver;

    }

    private Session session;

    private Session getSession() {

        if (this.session == null || !this.session.isLive()) {

            this.session = this.getResourceResolver().adaptTo(Session.class);

        }

        return this.session;

    }

    @Activate

    protected void activate(ComponentContext context) throws RepositoryException {

        this.resolver = this.getResourceResolver();

    }

    /**

     * Given the ID and type of a changed entity, search for all children of a given type 

     * under the changed entity and update the corresponding DB index.

     */

    public void updateIndexFor(

        String changedEntityId, 

        String changedEntityIdProp, 

        String changedEntitySearchPathRoot, 

        String childTypeToFind

    ) {

        Map<String, String> map;

        Query query;

        map = new HashMap<String, String>();

        map.put("path",             changedEntitySearchPathRoot);

        map.put("type",             childTypeToFind);

        map.put("property",         changedEntityIdProp);

        map.put("property.value",   changedEntityId);

        query = getQueryBuilder().createQuery(PredicateGroup.create(map), getSession());

        query.setHitsPerPage(0);

        SearchResult results = query.getResult();

        this.upload(results);

    }

    @Override

    public void upload(SearchResult results) {

        Map<String, List<DbIndexModel>> indexModels = new HashMap<String, List<DbIndexModel>>();

        DBIndexModel indexModel = null;

        String       indexName = null;

        Iterator<Resource> it = results.getResources();

        while (it.hasNext()) {

            Resource entity = it.next();

            try {

                switch (InventoryEntityTypeEnum.getEnum(entityType)) {

                    case BUILDING:

                        indexModel = modelFactory.createModel(entity, BuildingIndexModel.class);

                        indexName = "BuildingIndex";

                        break;

                    case SUITE:

                        indexModel = modelFactory.createModel(entity, SuiteIndexModel.class);

                        indexName = "SuiteIndex";

                        break;            

                    default:

                        return;

                }

                indexModels.add(indexModel);

            } catch (Exception e) {

                LOG.error("Exception creating model of entity type '{}' with ID '{}'...", entityType, nodeId);

                return;

            }

        }

        upload(indexModels, indexName); // mechanics of serializing models to JSON, sending data to DB

    }

}

/******** DB INDEX models -- aggregate fields from entity models (below) for updating external DB ***/

@Model(adaptables = Resource.class)

public class BuildingIndexModel extends BaseDbIndexModel {

    private MarketEntity            market;

    private SubmarketEntity         submarket;

    private PropertyEntity          property;

    private BuildingEntity          building;

    private OfficeLocationEntity    leasingOffice;

    @PostConstruct

    public void init() {

        try {

            // home/locations/{market}/{submarket}/{property}/jcr:content/buildings/{building}

            //                                         ^ parent   ^ parent    ^ parent ^ self

            this.building = self.adaptTo(BuildingEntity.class);

            //                             /{property}/jcr:content /buildings /{building} <- self

            Resource propertyPage   = self.getParent().getParent().getParent();

            this.property = propertyPage.getChild(JcrConstants.JCR_CONTENT).adaptTo(PropertyEntity.class);

            //                                     /{submkt}  /{property}  <- propertyPage

            Resource submarketPage  = propertyPage.getParent();

            this.submarket = submarketPage.getChild(JcrConstants.JCR_CONTENT).adaptTo(SubmarketEntity.class);

            Resource marketPage     = submarketPage.getParent();

            this.market = marketPage.getChild(JcrConstants.JCR_CONTENT).adaptTo(MarketEntity.class);

            // find closest non-empty leasingOfficePath in hierarchy from bottom to top

            String officePath = building.getLeasingOfficePath();

            if (StringUtils.isBlank(officePath)) { officePath = property  .getLeasingOfficePath(); }

            if (StringUtils.isBlank(officePath)) { officePath = submarket .getLeasingOfficePath(); }

            if (StringUtils.isBlank(officePath)) { officePath = market    .getLeasingOfficePath(); }

            this.leasingOffice = LeasingOfficeService.getOfficeFromPath(officePath, self.getResourceResolver());

        } catch (Exception e) {

            LOG.error("{} exception in BuildingIndexModel @PostConstruct method:", e.getCause());

        }

    }

    @Override public String getObjectID()   { return building.getNodeId(); }

    @Override public String getEntityType() { return building.getEntityType(); }

    @Override public String getName()       { return building.getName(); }

    @Override

    public Map<String, Object> getModel() {

        Map<String, Object> model = new HashMap<String, Object>();

        model.put("objectID",                       getObjectID());

        model.put("entityType",                     getEntityType());

        model.put("leasingOfficePath",              leasingOffice.getPath());

        model.put("leasingOfficeAddress",           leasingOffice.getAddress());

        model.put("leasingOfficePhone",             leasingOffice.getPhone());

        model.put("marketId",                       market.getMarketId());

        model.put("marketName",                     market.getMarketName());

        model.put("marketPagePath",                 market.getPath());

        model.put("marketThumbnailImages",          market.getThumbnailImageUrls());

        model.put("marketSearchDescription",        market.getSearchDescription());

        model.put("propertyId",                     property.getPropertyId());

        model.put("propertyName",                   property.getName());

        model.put("propertyPagePath",               property.getPath());

        model.put("propertyThumbnailImages",        property.getThumbnailImageUrls());

        model.put("hasCommonsArea",                 property.getHasCommonsArea());

        model.put("hasConferenceCenter",            property.getHasConferenceCenter());

        model.put("buildingId",                     building.getBuildingId());

        model.put("buildingName",                   building.getName());

        model.put("buildingStreetAddress",          building.getAddress());

        model.put("buildingPath",                   building.getPath());

        model.put("buildingThumbnailImages",        building.getThumbnailImageUrls());

        model.put("isEnergyStarCertified",          building.getIsEnergyStarCertified());

        return model;

    }

}

abstract class BaseDbIndexModel implements DbIndexModel {

    @Self protected Resource self;

    // dependencies

    @Reference protected ModelFactory modelFactory;

    public BaseDbIndexModel() {} // required parameterless constructor

    public String getModelInfo() {

        return String.format( "%s '%s' (id '%s')", getEntityType(), getName(), getObjectID() );

    }

    @Override

    public String toJsonString() {

        Gson gson = new GsonBuilder().setPrettyPrinting().disableHtmlEscaping().create();

        return gson.toJson( this.getModel() );

    }

}

/*********************************** Entity models -- extract data from JCR ***/

@Model(adaptables = Resource.class)

public class MarketEntity extends BaseLocationEntity {

    // /content/icop/ico/home/locations/{market} <- self

    // model fields from JCR:

    @Inject private String marketId;

    @Inject private String marketName;

    // expose the model:

    public String           getMarketId()               { return marketId; }

    public String           getMarketName()             { return marketName; }

    public String           getPath()                   { return self.getParent().getPath(); }  // <-- crashes, resolver is closed

    public Map<String, String> getThumbnailImageUrls()  { return getThumbnailImageUrls(self); }

    // fulfill interface contract

    @Override public String getName()       { return marketName; }

    @Override public String getNodeId()     { return marketId; }

}

@Model(adaptables = Resource.class)

public class BaseLocationEntity implements LocationEntity {

    @Self           protected Resource                      self;

    @SlingObject    protected ResourceResolver              resolver;

    @Inject @Optional   @Named("leasingOfficeLocationPath") private String leasingOfficePath;

    @Inject @Optional                                       private String  entityType;

    @Inject @Optional                                       protected String  name;

    @Inject @Optional                                       private String  nodeId;

    public String getLeasingOfficePath() { return leasingOfficePath; }    

    public String getPath() { return self.getPath(); }

    public String getModelInfo() { return String.format( "%s '%s' (id '%s')", getEntityType(), getName(), getNodeId()); }

}

[Shashi_Mulugu]

@valcohen avoid global variables for resourceresolver, try to declare it local to method scope and return the object when method invoked..  avoid using this for resourceresolver..

[valcohen]

Do you have any specific examples?I don't think I've seen this approach, can you show me how you'd change one of these classes to the style you recommend, or point me to some examples? Do I still inject the resolverFactory, but just call it in every method that needs it, and then close the resolver the method retrieved at the end of each method? How do I initialize the resolver in the Sling models in the absence of a SlingHttpRequest?

[Jörg_Hoh · Accepted solution]

The problem in in the MyScheduledTask class is that it is an OSGI component, and therefor a singleton. That means, that the resourceResolver field is shared amongst all invocations of the run() method. As this is a scheduled job, there is a chance that it is invoked (again) even if the previous invocation has not yet completed.
And that means, that at one point you close the ResourceResolver (and reopen it again), but then every reference to that now closed ResourceResolver is invalid, and you get the exception you are facing.
I would implement it like this:

   
    public void run() {
        try (ResourceResolver resourceResolver = getResourceResolver;){
            Resource msgFolder = resourceResolver.getResource("/path/to/message/nodes");
            [...]
        } catch (Exception e) {
            logger.error(e);
        } 
    }

In this case the ResourceResolver is private to every invocation, and you won't get that problem. Of course you need to pass the ResourceResolver then to every method.

In the marketEntity the problem is less obvious. I think that you use the same approach as above, but then you pass a Resource into a such a SlingModel, and that SlingModel lives longer than this ResourceResolver. In this example I would only store a simple String as the path property of the Model, not the resource itself (it does not bring any value).

[valcohen]

Hi Jörg, thanks for the reply! I'll try the try-with-resources approach -- it's a bit of a pain to pass the resolver in to all the methods, but I see your point about the singleton (the job runs once a minute, and if there's a heavy load of inbound messages the processing can definitely take longer than that). I assume I can just pass the resolver, and if I need to, adapt it to a QueryBuilder, PageManager, Session, etc. within the methods that need those objects, right? Or do I need to adapt them all when the resolver is created, and pass them separately?

I'm not clear on the comments about the Sling models, and that part is even harder for me to understand. The models need a resolver because some of their methods need to do traversal or queries. I may be able to precompute a path, but lots of other processing is encapsulated in the models. I currently inject the resolver into the Sling model base class, but don't really understand what resolver is being used there (what credentials does it carry?). Should I inject a ResolverFactory instead, and get a resolver inside the model's various methods? How can I manage its lifecycle to make sure it's closed when the model? Should I get a resolver in PostConstruct, and create a PreDestroy method to close it?

Thanks again for dropping in, I really appreciate the explanations and examples! 

[Jörg_Hoh]

Sling Models are normally created by adapting either a resource or a request to that SlingModel. So when it comes to injecting a ResourceResolver into a Sling Model class, there it's quite natural which ResourceResolver will be taken:

1. adapting from a resource: resource.getResourceResolver();
2. adapting from a request: request.getResourceResolver();

In the 1st case you need to make sure that the ResourceResolver is open as long as the model can be used (in the 2nd case this is implicitly the case). 

does that help?

Jörg


(Would be a nice topic for a blog post)

[valcohen]

Yes, that helps a lot, I'll give these techniques a try. My module is long overdue for a rewrite, I guess now is as good a time as any.

I'd love to read a blog about how you'd implement a design like this -- most of what I've seen is around adapting SlingHttpRequests and sending back the response, rather than adapting resources in background jobs. Thanks again!