On Sep 20, 2013, at 12:38 AM, Mark Thomas <ma...@apache.org> wrote: > On 20/09/2013 06:11, Jeremy Boynes wrote: >> On Sep 19, 2013, at 8:31 AM, Jeremy Boynes <jboy...@apache.org> wrote: >> >>> On Sep 19, 2013, at 6:07 AM, ma...@apache.org wrote: >>> >>>> Author: markt >>>> Date: Thu Sep 19 13:07:02 2013 >>>> New Revision: 1524727 >>>> >>>> URL: http://svn.apache.org/r1524727 >>>> Log: >>>> Always load container SCIs first >>> >>> This seems at odds with the requirement that "the order in which these >>> services are discovered MUST follow the application’s class loading >>> delegation model" which is independent of orderedLibs. If we base this on >>> ServiceLoader, which seems to be the intent, the algorithm would be: >>> >>> if (orderedLibs != null) >>> excludedJars = URLs of all jars in WEB-INF/lib that are not listed in >>> orderedLibs >>> for (URL configFile : webappLoader.getResources()) { >>> if (!excludedJars.contains(jar containing configFile)) { >>> load SCIs from entries in configFile >>> } >>> } >>> >>> I originally shied away from trying to compare the URLs returned by the >>> classloader and with those from ServletContext.getResource() as I was was >>> not sure they would be the same. If they are, then the implementation can >>> be simplified along the lines above. If not, we would also need to pass in >>> the delegation order of the classloader to know whether to add the >>> "application" SCIs before or after the "container" ones. >>> >>> I'll go ahead and modify this based on the assumption the URLs can be >>> compared. >> >> Patch that changes this to load in the order returned by the ClassLoader > > -1 > > a) The container defined SCIs need to be loaded first. Any other > solution places an unnecessary burden on application developers. It is > reasonable for application developers to expect that if a container > advertises support for WebSocket, JSP, etc. then those features are > available when the application's SCI's are executed.
I respectfully disagree with this. The spec consistently refers to SCIs as a mechanism for extending the container and not as an application API. The examples it contains refer to frameworks such as "JAX-WS, JAX-RS and JSF" not application code; and there is, of course, the whole section on JSP pluggability. The HandlesTypes mechanism allows frameworks to configure servlet components (servlets, filters, listeners) based on annotations on application code; it is not intended for application code itself. The registration API allows components to be "preliminary" when SCIs are executing and does not guarantee they are available. And so on... IOW, the ambiguity in SCI ordering is an issue for framework developers not application developers. Further, in the original user's case they were trying to access a WebSocket ServerContainer. To quote the WebSocket specification: > "When running on the web container, the addEndpoint methods may be called > from a javax.servlet.ServletContextListener provided by the developer and > configured in the deployment descriptor of the web application." it specifically refers to use in a listener rather than an SCI. IMO, this is an application bug, one that is easily corrected by following the WebSocket spec and which does not need container changes. > b) The URls are not (yet) comparable. Further refactoring of the new > resources implementation is required before this is true in all cases. > > c) The Servlet specification currently makes no statement on a required > execution order - therefore any ordering scheme is specification > compliant. From a specification point of view, both the current code and > the proposed patch are acceptable. The only ordering concern for SCIs in the spec is that they are "discovered" following the classloader delegation model. This will typically be configured to load application classes first, something r1524727 does not do. We may choose to invoke SCIs in a different order, but we need to "discover" them correctly. If we are going to provide an ordering mechanism for SCIs, then it needs to be more robust to support cases such as Remy's where there are ordering rules at a finer granularity, such as JSP before WebSocket or vice versa. Something similar but separate to fragment ordering that applies specifically to SCIs. > d) No-one on the EG has yet raised any objections to the proposal to > require container defined SCIs to be loaded first. I am not privy to EG discussions so cannot comment. If I was participating, I would raise the same objections there. > I have plans to address b) as it should allow the simplification of the > resources handling in the WebappClassLoader but even with that issue > addressed the other points above stand. As I've said, I don't like any mechanism relying on matching URLs either. The exclusion cause for SCI discovery is ambiguous and a weird coupling of the fragment ordering mechanism with SCI service loading and if anything I would hope the EG re-evaluates that in 3.2 as well, especially as they expect that to be based on the semantics of the ServiceLoader mechanism. Cheers Jeremy
signature.asc
Description: Message signed with OpenPGP using GPGMail
