SLING-5668 - Leverage ServletRequestListener.requestDestroyed for cal… #1
Conversation
…ling DisposalCallback in case the model was created from a request
| @@ -1046,11 +1075,22 @@ protected ThreadInvocationCounter initialValue() { | |||
|
|
|||
| this.configPrinterRegistration = bundleContext.registerService(Object.class.getName(), | |||
| new ModelConfigurationPrinter(this, bundleContext, adapterImplementations), printerProps); | |||
|
|
|||
| Hashtable<Object, Object> listenerProps = new Hashtable<>(); | |||
There was a problem hiding this comment.
What about registering this service via DS? Seems easier and requires less code IMHO.
There was a problem hiding this comment.
The problem with DS in this case (which is already true of the ConfigurationPrinter right above this) is that it needs access to internal data structures of the ModelAdapterFactory, so I'd rather keep that all internal and manage the lifecycle manually.
There was a problem hiding this comment.
If I understand correctly all service properties are constants. If you add the ServletRequestListener to the Service annotation from above, the same component (i.e. object/instance) would be used for both the ModelAdapterFactory and the ServletRequestListener so you would not have any problems access internal data structure of ModelAdapterFactory from ServletRequestListener.onDispose. The only drawback I see is that you need to combine/expose the properties for both services (i.e. ModelAdapterFactory and ServletRequestListener) in this single component.
There was a problem hiding this comment.
Ah, I see what you mean now. Yeah, that might work.
| @@ -1256,4 +1300,16 @@ private Object handleBoundModelResult(Result<?> result) { | |||
| scriptEngineFactory, bindingsValuesProvidersByContext).adaptTo(targetClass); | |||
| } | |||
|
|
|||
| @Override | |||
| public void requestDestroyed(ServletRequestEvent sre) { | |||
| ServletRequest request = unwrapRequest(sre.getServletRequest()); | |||
There was a problem hiding this comment.
I also find it weird, that this event does still contained a wrapped servlet request. But unwrapping does not do any harm (in no case).
| @@ -1046,11 +1075,22 @@ protected ThreadInvocationCounter initialValue() { | |||
|
|
|||
| this.configPrinterRegistration = bundleContext.registerService(Object.class.getName(), | |||
| new ModelConfigurationPrinter(this, bundleContext, adapterImplementations), printerProps); | |||
|
|
|||
| Hashtable<Object, Object> listenerProps = new Hashtable<>(); | |||
| listenerProps.put("osgi.http.whiteboard.context.select", "(osgi.http.whiteboard.context.name=*)"); | |||
There was a problem hiding this comment.
Could you use the constants from http://javadox.com/org.osgi/osgi.cmpn/6.0.0/org/osgi/service/http/whiteboard/HttpWhiteboardConstants.html.
There was a problem hiding this comment.
Not without bumping the dependency. But maybe that should be done anyway since (as far as I know), this functionality will effectively depend upon a 6.0 HttpService implementation.
There was a problem hiding this comment.
Yes, bumping the dependency is reasonable as HTTP whiteboard is only available since OSGi Comp R6.
|
An IT is a good idea, although I'm not sure how to do it since the request would be synthetic and thus not fire the listener. A unit test should be possible. I'll look into that. |
… implement unit test
|
I've made the requested changes. |
…ling DisposalCallback in case the model was created from a request