Stefano, this resource leak is certainly worth fixing. I have a few thoughts:
(1) app-schema supports cyclic references between features of different type. For example, a feature of type A can have nested within it a feature of type B which has nested within it the containing feature (there are examples of this in GeoSciML). Some schemas may have cyclic types between different features instances (I imagine something like an adjacentFeature property). Or we can have A contains B contains C which contains A, and so on. Any or all of these can be top-level accessible types, and several could be non-feature types. The encoder sorts this out by encoding the first instance by value and repeated instances by reference, to preserve the uniqueness of gml:id (I do not know how deep across cycles filtering is supported). Do cyclic type relationships change your implementation strategy? (2) The language in the code and manual is "non-feature" types. I wonder if this covers all situations? Do you need a flag or can you use instanceof? Should the DataAccessRegistry create and maintain a directed dependency graph? It may not in general be a tree. (3) Can internal non-feature types be shared between accessible types? (4) DataAccessRegistry is an ugly, ugly hack. It contains a static singleton, and exists because there is no GeoTools API by which a DataAccess can discover other DataAccesses that it might need to respond to queries. I could not think of a better way of doing it without adding a new manager interface and modifying GeoServer to use it [1]. This was never a problem before you added REST support, because all DataAccesses were started and registered in a single GeoServer thread at startup time. Now you have added REST support, and no good deed goes unpunished. Hopefully catalog locking will protect against concurrent modification, but the architecture of DataAccessRegistry would benefit from a redesign. In short, please consider refactoring this class; I am also happy for you to adjust it in a simple way like that you described because the cost of refactoring may not be worth the benefit. [1] JEE architecture in a nutshell, Spring I am looking at you: http://discuss.joelonsoftware.com/default.asp?joel.3.219431.12 Kind regards, Ben. On 09/09/15 02:43, Stefano Costa wrote: > Dear developers, > I have a workflow in which: > 1. an app-schema mapping configuration involving multiple files is used to > create an app-schema data store via REST as per this example: > http://docs.geoserver.org/latest/en/user/rest/examples/curl.html#uploading-multiple-app-schema-mapping-files > 2. the data store created in 1. is deleted and created again with a > different mapping configuration (same mapped types, different attribute > mappings) > > ----> a DataSourceException is thrown: "Duplicate mappingName or > targetElement across FeatureTypeMapping instances detected..." > > I traced the problem to the fact that my configuration is mapping > non-feature types separately > <http://docs.geoserver.org/latest/en/user/data/app-schema/mapping-file.html#includedtypes-optional>: > when > the configuration is parsed, two distinct AppSchemaDataAccess instances are > registered, one for the "real" datastore, another not directly accessible > (does not appear in GetCapabilities nor GetFeature requests): > https://github.com/geotools/geotools/blob/64bca8b6c87655b8f54c5e4b02b48e4ca371ffe7/modules/extension/app-schema/app-schema/src/main/java/org/geotools/data/complex/AppSchemaDataAccessFactory.java#L93 > > ...but only one is removed from the data access registry on dispose: > https://github.com/geotools/geotools/blob/64bca8b6c87655b8f54c5e4b02b48e4ca371ffe7/modules/extension/app-schema/app-schema/src/main/java/org/geotools/data/complex/AppSchemaDataAccess.java#L598 > https://github.com/geotools/geotools/blob/64bca8b6c87655b8f54c5e4b02b48e4ca371ffe7/modules/extension/app-schema/app-schema/src/main/java/org/geotools/data/complex/DataAccessRegistry.java#L309 > > So, on the second creation attempt, non-feature types are still registered > and a "duplicate mappingName or targetElement" exception is thrown. > > The issue does not occur if GeoServer is restarted after deleting the data > store and before attempting to create it again, of course: that's likely > the reason why this issue never came up before the introduction of the > app-schema REST API, back in the days when app-schema data stores could > only be configured manually and required a restart to be noticed anyway. > > To solve this inconvenience, I propose to modify AppSchemaDataAccess as > such: > - add a flag, called hidden or notAccessible, to mark AppSchemaDataAccess > instances that should be automatically removed when no longer needed > - add a couple of methods, addRelatedDataAccessInstance(DataAccess > dataAccess) and Set<DataAccess> getRelatedDataAccessInstances(), to keep > track of the related types needed by each "real" AppSchemaDataAccess > instance > - add some logic in DataAccessRegistry.unregister(DataAccess dataAccess) to > automatically remove hidden or notAccessible AppSchemaDataAccess instances > that are no longer referenced > > If you think my approach is sound, I can prepare a PR. > > > Thanks for any feedback! > > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > GeoTools-Devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/geotools-devel > -- Ben Caradoc-Davies <[email protected]> Director Transient Software Limited <http://transient.nz/> New Zealand ------------------------------------------------------------------------------ _______________________________________________ GeoTools-Devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
