Rafael,
On Wed, Sep 18, 2013 at 3:54 PM, Rafael W. <[email protected]> wrote: > Hello everybody, > > Partially, I understand the hesitation since this also targets the public > API. On the other side, I still think that the change would be appropriate. > So hear me out on this. My current work around for using ResourceModels > from worker threads is a simple wrapper: > > public class LazyModelEval implements Serializable { > private final IModel<?> model; > public LazyModelEval(IModel<?> model) { this.model = model; } > @Override public String toString() { return model.getObject().toString(); > } > } > > By implementing the Serializable interface, I fulfill the signature of the > feedback message methods such as error and by the custom implementation of > the toString method, I guarantee that other components in my company’s > application that use the default FeedbackPanel and were written before my > code do not need to be changed, since the FeedbackPanel component calls > toString on all non-null feedback messages. > > Thanks to that, I can use an error message model from a current thread: > > error(new LazyModelEval(new ResourceModel(“my.key”))); > > because the model is evaluated lazily when the message is actually > displayed and an application is by definition attached to the evaluating > thread. My argument is however that this work-around should not be > necessary but should instead come out of the box. Essentially, I do not > suggest an API change (aside, its more an API extension since I kept my > suggestion fully backwards compatible), but I suggest a change of internal > representation. Seeing it from an architect’s point of view, I believe, the > following two things should be separate: > > 1. 1. An error occurs: at this point the error should be defined in a > most general fashion, by example via a ResourceModel since Wicket already > knows a type of abstraction here. This kind of reporting can occur at any > place that performs some business logic. In my case, the business logic is > performed throughout many threads which originate from a shared thread pool > and which do not need to be attached to the running Wicket application. > > 2. 2. An error is to be reported / displayed to the user: The error > message should be resolved from its abstract description and also localized > (all this might even never happen). This will in general happen in the UI > thread such that resolving a ResourceModel is in general safe. > > Until today, 95% of the feedback I get for my component is that it does not > work as expected, because users of Wicket do in general not understand its > internal structure and that the request handler thread is somewhat special > since it has some “thread global” variables attached to it. Therefore, > people trigger error models from the background thread as if they are > reported in the UI thread and do not understand why this causes a runtime > exception: > > error(new ResourceModel(“my.key”).getObject()); > > However, if a signature would already point to > > error(new ResourceModel(“my.key”)); > The problem is not in the signature of #error/success/warn/... As we said several times you can do error(new ResourceModel(“my.key”)); even now. All you need to do additionally is to extend FeedbackPanel to be able to get the model's object when rendering the messages. The problem is in the multithreading. By using a different thread there is no guarantee that your call to #error(someModel) will be before FeedbackPanel#render() (executed in the http worker thread). At the moment Wicket doesn't use Servlet's 3.0 AsyncContext so the http worker thread will not wait for your custom threads. By using FutureModel you can block on #getObject() and verify that all is OK. > > this could be done without an exception. Again, this representation makes > more sense to me in the first place. Until today, I explain to people how > to use the solution with the LazyModelEval object and they take it what > makes the problem go away. However, after open sourcing my solution, other > people will most likely run into the same problem when using my component. > (On a side note, I think multithreading in web application is an important > issue and will become even more important in the future. Think about the > Play! framework and its Akka actors.) > Servlet 3.0 AsyncContext provides something similar. Servlet 3.1 async read/write listeners gives something similar to Play's Iteratees. > > All that said: This is why I tagged the suggested change as an improvement, > not as a bug. It would allow people to use multithreading in Wicket in a > more natural fashion. In general, background threads do some job and report > some feedback after the job is done. I think this change would therefore > cover most use cases of multithreading and also ease the use of Wicket. > Also, I thought the change is rather minor and very cheap in measures of > lines of codes. > > Finally, I feel like most of the feedback I get on this is more concerned > about how I could get around this problem or with how the change could be > implemented with changing fewer lines of code. I know that this is > possible, thanks to the general signature of the feedback methods such as > error to get things done by for example type casting. However, type casting > can achieve a lot of things, but usually it does not make programs more > intuitive. When I implemented the change I thought more of: How can I make > the greatest use of type safety without breaking backwards compatibility > here and this is what I come up with. Also, I think it comes very much in > favor with the principle of least surprise since people can simply register > a model as an error message without thinking about its resolution. > > Again, all this is just a suggestion and I know for myself how to handle > the problem. I do however still think this change would make Wicket easier > to use and might safe some people frustration. You might want to have a > look at my quick start app for understanding why. > > But of course, in the end, it’s up to you. If you finally do not want the > change, I will just close my pull request from GitHub in the course of the > week. > > Best, Rafael >
