> Emmanuel Lecharny [mailto:[EMAIL PROTECTED] wrote: > > Steve Ulrich wrote: > > > > An init() method doesn't know anything about the session so far. > > > I was specifically thinking if a init(session) method. We also need a > destroy(session) method. Eh, call them preAdd and postRemove ;)
Okay :-) > Seriously, you make a good point here, and enlighted the reason we have > these methods. And after a day looking at those preAdd and postRemove > (well, not a full day !), I think it's better to have those two guys > called directly when you do a add() and remove() than to have an > explicit init() method, generally speaking. But there is another Hmm.. dedicated init and destroy methods would help to make the code clean. If they don't exists, some will call it preAdd/postRemove, others init/destroy, beforeInsert/afterRemove... Additional (and increasing with the number of method-pair-names) there's a possibility to forget to call the methods and just doing add/remove and getting errors. > problem : > > If you look at the existing javadoc : > > /** > * Invoked by [EMAIL PROTECTED] ReferenceCountingFilter} when this filter > * is added to a [EMAIL PROTECTED] IoFilterChain} at the first time, so > you > can > * initialize shared resources. Please note that this method is > never > * called if you don't wrap a filter with [EMAIL PROTECTED] > ReferenceCountingFilter}. > */ > void init() throws Exception; ouch! > /** > * Invoked before this filter is added to the specified > <tt>parent</tt>. > * Please note that this method can be invoked more than once if > * this filter is added to more than one parents. This method is > not > * invoked before [EMAIL PROTECTED] #init()} is invoked. > */ > void onPreAdd(IoFilterChain parent, String name, NextFilter > nextFilter) > throws Exception; > > You can see that the init is used in a very specific case : when you > wrap the filter with ReferenceCountingFilter (seems like an abusive > usage to me), and that the onPreAdd may be called more than once, which > seems also abusive when you know that we will only have one instance of > this filter, leading potentially to many mistakes if the initialization > done during this phase is done more than once. > so : I think that we need to rethink about what those two methods do, > in > order to protect the user against dangerous hidden mistakes... IMO, the normal case is that a filter will only be added once. If they're added more than one time, the filter must be responsible for that. Maybe an additional interface can provide the neccessary methods to handle that case. If the Filter does not implement it, it can only be added once. This way, the "if (chain.contains(this))" could be removed from the filter implementations. > I'm going to propose something slightly better for those methods : > - remove the postAdd and preRemove methods > - rename the preAdd/postRemove to init(session) and destroy(session), agreed > and call them explicitly _before_ we remove or add this filter from the > session chain see above comment. > - if we don't need those initialization/destruction, then default to a > flag being set indication the filter's status (initialized/removed) > - throw an exception if the status is incorrect when he use the filter > (protecting the user if he forgets to initialize the filter). Sounds a bit comlicated from a users point of view. Maybe we can use an interface here, too. The add and remove method can check if the filter implements the interface and call the method as needed. > I'm not completely convinced that I have the best possible idea in this > area, I'm just trying to be sure that we don't miss something important > here. feel free to comment and tell if I'm wrong ! regards Steve
