As a follow up:
error(new ResourceModel(“my.key”).getObject());
error(new ResourceModel(“my.key”));
Both might/will fail if called from a non-request thread, because there
is no Application attached.
Sven
On 09/18/2013 02:54 PM, Rafael W. 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”));
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.)
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
