Just FYI, I implemented the simplified logic for Tapestry/Shiro integration as described. I'm quite happy how it turned out and in the process I was able to obsolete big chunks of path handling code (for example, see http://svn.codehaus.org/tynamo/trunk/tapestry-security/src/test/java/org/tynamo/security/testapp/services/AppModule.java). I think the same approach would be quite valuable to the Shiro community at large as well, though I personally have perhaps less incentive now to push the changes upstream. Since the path matching and path specific configuration is specific to Shiro filters, the concerns Les raised earlier don't quite apply - obviously you can share filter instances between different filter chains, it's just that chain specific configuration is much simpler (and will perform better) if you don't. If we decide to keep the (Shiro) issue open, perhaps we can bring the topic up again when planning for 2.0.
Kalle On Sat, Feb 12, 2011 at 5:10 PM, Kalle Korhonen <[email protected]> wrote: > On Sat, Feb 12, 2011 at 3:25 PM, Les Hazlewood <[email protected]> wrote: >> Actually, in thinking about this more, I don't think we should create >> new Filter instances per chain by default, and here's why: >> 1. Shiro filter chains can contain _any_ javax.servlet.Filter > > Sure, which may the be exact problem. By the servlet spec, the filters > are configured in web.xml. We've talked about that before and Shiro > would also be a better fit for Play framework if these filters weren't > servlet filters. > >> 2. Creating a new Filter instance per chain can be a dangerous thing >> to do - there are many filters that, upon startup/init, go through > > Sure it could, but that's certainly addressable if there were any such > filters with significant initialization logic. Even then, it's > one-time cost instead of recurring cost. > >> I don't know what Tapestry Security supports, but if you're allowed to >> configure any javax.servlet.Filter like you can in Shiro's INI, I > > Currently it does, but I just opened an issue for it and specifically > noted that they shouldn't be allowed anymore > (http://jira.codehaus.org/browse/TYNAMO-76). BTW, Tapestry is > implemented as a filter as well, so obviously you can use *standard* > servlet filters together with Tapestry as you see fit. > >> Finally, and please correct me if I'm wrong, but your main concern in >> this thread isn't really about new instances vs pooled instances (if >> it was, I'm assuming you'd argue that the Servlet containers are doing >> it incorrectly as well, since we just retain the same behavior). It >> is almost entirely driven by your concern about performance due to >> executing path matching too often, correct? > > Right about the performance, that's what the PathMatchingFilter issue > all is about. Neither issue I opened says anything about the pool > either way. Absolutely wrong assumption on the servlet container > behavior: that comparison is off - there's no behavior such as filter > chains. By the spec, each filter includes the mapping configuration in > itself and they are all being compared to, similar to executing them > all as separate chains. > > As is, please provide closing comments to > https://issues.apache.org/jira/browse/SHIRO-256 for future reference. > > Kalle >
