You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Martijn Dashorst <ma...@gmail.com> on 2014/11/24 23:39:48 UTC
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
It appears innocent enough this change, but we can't be sure this
won't break applications: isAttached() is a public method and it is
valid to call inside load(). If someone depends on isAttached() during
load() we will change this behavior.
Granted this new semantics are much more to my liking, and it fixes a
bug. But does this justify the risk of breaking applications in other
ways?
I checked some of our projects for such mishaps but couldn't find any
misuses of isAttached inside load().
Martijn
On Mon, Nov 24, 2014 at 9:11 PM, Sven Meier (JIRA) <ji...@apache.org> wrote:
>
> [ https://issues.apache.org/jira/browse/WICKET-5772?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
>
> Sven Meier resolved WICKET-5772.
> --------------------------------
> Resolution: Fixed
> Fix Version/s: 6.19.0
> 7.0.0-M5
>
> #attached is now set after calling #load(), as suggested.
>
> Thanks Martin!
>
>> LoadableDetachableModel caches null value if load() fails, bug in getObject() {attached = true;}
>> ------------------------------------------------------------------------------------------------
>>
>> Key: WICKET-5772
>> URL: https://issues.apache.org/jira/browse/WICKET-5772
>> Project: Wicket
>> Issue Type: Bug
>> Components: wicket
>> Affects Versions: 1.4.23, 6.18.0, 7.0.0-M4
>> Reporter: Martin Makundi
>> Assignee: Sven Meier
>> Labels: easyfix, easytest, patch
>> Fix For: 7.0.0-M5, 6.19.0
>>
>> Original Estimate: 10m
>> Remaining Estimate: 10m
>>
>> If you have a LoadableDetachableModel whose load() operation fails at an instance, the LoadableDetachableModel will cache null value because
>> getObject() method sets attached = true; before load() invocation has returned.
>> This results in methods trusting LoadableDetachableModel using the null value for their operations which is logically incorrect as null might not be a legal value at all. Such behavior would result in unexpected difficult-to-debug behavior in depending components.
>> Easy fix:
>> Move:
>> attached = true;
>> after line:
>> transientModelObject = load();
>> Test case:
>> /**
>> * LoadableDetachableModel must not return null as an attached value if load()
>> * fails.
>> */
>> public void testWhenLoadFails() {
>> final LoadableDetachableModel<Void> loadableDetachableModel = new LoadableDetachableModel<Void>() {
>> /**
>> * @see org.apache.wicket.model.LoadableDetachableModel#load()
>> */
>> @Override
>> protected Void load() {
>> throw new UnsupportedOperationException("Let's assume this fails for some reason.");
>> }
>> };
>> try {
>> loadableDetachableModel.getObject();
>> fail(UnsupportedOperationException.class + " expected.");
>> } catch (final UnsupportedOperationException e) {
>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected behavior due to " + UnsupportedOperationException.class);
>> }
>> try {
>> assertNotSame(LoadableDetachableModel.class + " should not return null if it's load() has failed\n", null,
>> loadableDetachableModel.getObject());
>> } catch (final UnsupportedOperationException e) {
>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected behavior due to " + UnsupportedOperationException.class);
>> }
>> }
>
>
>
> --
> This message was sent by Atlassian JIRA
> (v6.3.4#6332)
--
Become a Wicket expert, learn from the best: http://wicketinaction.com
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Martijn Dashorst <ma...@gmail.com>.
I've created a jira and fixed the issue:
https://issues.apache.org/jira/browse/WICKET-5916
Martijn
On Fri, May 29, 2015 at 6:27 PM, Martijn Dashorst
<ma...@gmail.com> wrote:
> Apparently this change breaks our application after all: a
> StackOverflowError trying to load a ResourceModel from a DDC's
> "appendOptionHtml" method.
>
> JaNeeCombobox.getResourcePostfix() line: 106
> CobraLocalizer.getCacheKey(String, Component, Locale, String, String) line: 24
> CobraLocalizer(Localizer).getStringIgnoreSettings(String, Component,
> IModel<?>, Locale, String, String) line: 369
> CobraLocalizer(Localizer).getString(String, Component, IModel<?>,
> Locale, String, IModel<String>) line: 232
> ResourceModel$AssignmentWrapper.load() line: 132
> ResourceModel$AssignmentWrapper.load() line: 1
> ResourceModel$AssignmentWrapper(LoadableDetachableModel<T>).getObject()
> line: 120
> JaNeeCombobox.getResourcePostfix() line: 114
> CobraLocalizer.getCacheKey(String, Component, Locale, String, String) line: 24
> CobraLocalizer(Localizer).getStringIgnoreSettings(String, Component,
> IModel<?>, Locale, String, String) line: 369
> CobraLocalizer(Localizer).getString(String, Component, IModel<?>,
> Locale, String, IModel<String>) line: 232
> CobraLocalizer(Localizer).getString(String, Component, IModel<?>,
> Locale, String, String) line: 201
> CobraLocalizer(Localizer).getString(String, Component, String) line: 150
> JaNeeCombobox$JaNeeRenderer.getDisplayValue(Boolean) line: 84
> JaNeeCombobox$JaNeeRenderer.getDisplayValue(Object) line: 1
> JaNeeCombobox(AbstractChoice<T,E>).appendOptionHtml(AppendingStringBuffer,
> E, int, String) line: 409
>
> This worked until this change came in. It appears that the LDM should
> be extended with an additional flag "attaching" to break load cycles.
> While there's probably an issue in our code base, this likely will
> cause issues for others upgrading as well.
>
> Martijn
>
>
>
> On Mon, Nov 24, 2014 at 11:39 PM, Martijn Dashorst
> <ma...@gmail.com> wrote:
>> It appears innocent enough this change, but we can't be sure this
>> won't break applications: isAttached() is a public method and it is
>> valid to call inside load(). If someone depends on isAttached() during
>> load() we will change this behavior.
>>
>> Granted this new semantics are much more to my liking, and it fixes a
>> bug. But does this justify the risk of breaking applications in other
>> ways?
>>
>> I checked some of our projects for such mishaps but couldn't find any
>> misuses of isAttached inside load().
>>
>> Martijn
>>
>>
>> On Mon, Nov 24, 2014 at 9:11 PM, Sven Meier (JIRA) <ji...@apache.org> wrote:
>>>
>>> [ https://issues.apache.org/jira/browse/WICKET-5772?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
>>>
>>> Sven Meier resolved WICKET-5772.
>>> --------------------------------
>>> Resolution: Fixed
>>> Fix Version/s: 6.19.0
>>> 7.0.0-M5
>>>
>>> #attached is now set after calling #load(), as suggested.
>>>
>>> Thanks Martin!
>>>
>>>> LoadableDetachableModel caches null value if load() fails, bug in getObject() {attached = true;}
>>>> ------------------------------------------------------------------------------------------------
>>>>
>>>> Key: WICKET-5772
>>>> URL: https://issues.apache.org/jira/browse/WICKET-5772
>>>> Project: Wicket
>>>> Issue Type: Bug
>>>> Components: wicket
>>>> Affects Versions: 1.4.23, 6.18.0, 7.0.0-M4
>>>> Reporter: Martin Makundi
>>>> Assignee: Sven Meier
>>>> Labels: easyfix, easytest, patch
>>>> Fix For: 7.0.0-M5, 6.19.0
>>>>
>>>> Original Estimate: 10m
>>>> Remaining Estimate: 10m
>>>>
>>>> If you have a LoadableDetachableModel whose load() operation fails at an instance, the LoadableDetachableModel will cache null value because
>>>> getObject() method sets attached = true; before load() invocation has returned.
>>>> This results in methods trusting LoadableDetachableModel using the null value for their operations which is logically incorrect as null might not be a legal value at all. Such behavior would result in unexpected difficult-to-debug behavior in depending components.
>>>> Easy fix:
>>>> Move:
>>>> attached = true;
>>>> after line:
>>>> transientModelObject = load();
>>>> Test case:
>>>> /**
>>>> * LoadableDetachableModel must not return null as an attached value if load()
>>>> * fails.
>>>> */
>>>> public void testWhenLoadFails() {
>>>> final LoadableDetachableModel<Void> loadableDetachableModel = new LoadableDetachableModel<Void>() {
>>>> /**
>>>> * @see org.apache.wicket.model.LoadableDetachableModel#load()
>>>> */
>>>> @Override
>>>> protected Void load() {
>>>> throw new UnsupportedOperationException("Let's assume this fails for some reason.");
>>>> }
>>>> };
>>>> try {
>>>> loadableDetachableModel.getObject();
>>>> fail(UnsupportedOperationException.class + " expected.");
>>>> } catch (final UnsupportedOperationException e) {
>>>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected behavior due to " + UnsupportedOperationException.class);
>>>> }
>>>> try {
>>>> assertNotSame(LoadableDetachableModel.class + " should not return null if it's load() has failed\n", null,
>>>> loadableDetachableModel.getObject());
>>>> } catch (final UnsupportedOperationException e) {
>>>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected behavior due to " + UnsupportedOperationException.class);
>>>> }
>>>> }
>>>
>>>
>>>
>>> --
>>> This message was sent by Atlassian JIRA
>>> (v6.3.4#6332)
>>
>>
>>
>> --
>> Become a Wicket expert, learn from the best: http://wicketinaction.com
>
>
>
> --
> Become a Wicket expert, learn from the best: http://wicketinaction.com
--
Become a Wicket expert, learn from the best: http://wicketinaction.com
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Martijn Dashorst <ma...@gmail.com>.
Apparently this change breaks our application after all: a
StackOverflowError trying to load a ResourceModel from a DDC's
"appendOptionHtml" method.
JaNeeCombobox.getResourcePostfix() line: 106
CobraLocalizer.getCacheKey(String, Component, Locale, String, String) line: 24
CobraLocalizer(Localizer).getStringIgnoreSettings(String, Component,
IModel<?>, Locale, String, String) line: 369
CobraLocalizer(Localizer).getString(String, Component, IModel<?>,
Locale, String, IModel<String>) line: 232
ResourceModel$AssignmentWrapper.load() line: 132
ResourceModel$AssignmentWrapper.load() line: 1
ResourceModel$AssignmentWrapper(LoadableDetachableModel<T>).getObject()
line: 120
JaNeeCombobox.getResourcePostfix() line: 114
CobraLocalizer.getCacheKey(String, Component, Locale, String, String) line: 24
CobraLocalizer(Localizer).getStringIgnoreSettings(String, Component,
IModel<?>, Locale, String, String) line: 369
CobraLocalizer(Localizer).getString(String, Component, IModel<?>,
Locale, String, IModel<String>) line: 232
CobraLocalizer(Localizer).getString(String, Component, IModel<?>,
Locale, String, String) line: 201
CobraLocalizer(Localizer).getString(String, Component, String) line: 150
JaNeeCombobox$JaNeeRenderer.getDisplayValue(Boolean) line: 84
JaNeeCombobox$JaNeeRenderer.getDisplayValue(Object) line: 1
JaNeeCombobox(AbstractChoice<T,E>).appendOptionHtml(AppendingStringBuffer,
E, int, String) line: 409
This worked until this change came in. It appears that the LDM should
be extended with an additional flag "attaching" to break load cycles.
While there's probably an issue in our code base, this likely will
cause issues for others upgrading as well.
Martijn
On Mon, Nov 24, 2014 at 11:39 PM, Martijn Dashorst
<ma...@gmail.com> wrote:
> It appears innocent enough this change, but we can't be sure this
> won't break applications: isAttached() is a public method and it is
> valid to call inside load(). If someone depends on isAttached() during
> load() we will change this behavior.
>
> Granted this new semantics are much more to my liking, and it fixes a
> bug. But does this justify the risk of breaking applications in other
> ways?
>
> I checked some of our projects for such mishaps but couldn't find any
> misuses of isAttached inside load().
>
> Martijn
>
>
> On Mon, Nov 24, 2014 at 9:11 PM, Sven Meier (JIRA) <ji...@apache.org> wrote:
>>
>> [ https://issues.apache.org/jira/browse/WICKET-5772?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
>>
>> Sven Meier resolved WICKET-5772.
>> --------------------------------
>> Resolution: Fixed
>> Fix Version/s: 6.19.0
>> 7.0.0-M5
>>
>> #attached is now set after calling #load(), as suggested.
>>
>> Thanks Martin!
>>
>>> LoadableDetachableModel caches null value if load() fails, bug in getObject() {attached = true;}
>>> ------------------------------------------------------------------------------------------------
>>>
>>> Key: WICKET-5772
>>> URL: https://issues.apache.org/jira/browse/WICKET-5772
>>> Project: Wicket
>>> Issue Type: Bug
>>> Components: wicket
>>> Affects Versions: 1.4.23, 6.18.0, 7.0.0-M4
>>> Reporter: Martin Makundi
>>> Assignee: Sven Meier
>>> Labels: easyfix, easytest, patch
>>> Fix For: 7.0.0-M5, 6.19.0
>>>
>>> Original Estimate: 10m
>>> Remaining Estimate: 10m
>>>
>>> If you have a LoadableDetachableModel whose load() operation fails at an instance, the LoadableDetachableModel will cache null value because
>>> getObject() method sets attached = true; before load() invocation has returned.
>>> This results in methods trusting LoadableDetachableModel using the null value for their operations which is logically incorrect as null might not be a legal value at all. Such behavior would result in unexpected difficult-to-debug behavior in depending components.
>>> Easy fix:
>>> Move:
>>> attached = true;
>>> after line:
>>> transientModelObject = load();
>>> Test case:
>>> /**
>>> * LoadableDetachableModel must not return null as an attached value if load()
>>> * fails.
>>> */
>>> public void testWhenLoadFails() {
>>> final LoadableDetachableModel<Void> loadableDetachableModel = new LoadableDetachableModel<Void>() {
>>> /**
>>> * @see org.apache.wicket.model.LoadableDetachableModel#load()
>>> */
>>> @Override
>>> protected Void load() {
>>> throw new UnsupportedOperationException("Let's assume this fails for some reason.");
>>> }
>>> };
>>> try {
>>> loadableDetachableModel.getObject();
>>> fail(UnsupportedOperationException.class + " expected.");
>>> } catch (final UnsupportedOperationException e) {
>>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected behavior due to " + UnsupportedOperationException.class);
>>> }
>>> try {
>>> assertNotSame(LoadableDetachableModel.class + " should not return null if it's load() has failed\n", null,
>>> loadableDetachableModel.getObject());
>>> } catch (final UnsupportedOperationException e) {
>>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected behavior due to " + UnsupportedOperationException.class);
>>> }
>>> }
>>
>>
>>
>> --
>> This message was sent by Atlassian JIRA
>> (v6.3.4#6332)
>
>
>
> --
> Become a Wicket expert, learn from the best: http://wicketinaction.com
--
Become a Wicket expert, learn from the best: http://wicketinaction.com
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Martin Grigorov <mg...@apache.org>.
On Tue, Nov 25, 2014 at 1:11 AM, Sven Meier <sv...@meiers.net> wrote:
> Hi Martin,
>
> > isAttached() is a public method and it is valid to call inside load()
>
> that's a valid point to consider: But what purpose could it have to call
> isAttached() in load()?
> It would return <true> during the complete invocation.
>
> Since Wicket allows a single thread only to access the component tree, I
> don't see a race condition either.
>
Another possible problem:
Wicket synchronizes on page instance, but there is no guarantee that a
Model is not shared between several instances.
Or the LDM could be used in a shared IResource.
With the change it is possible that two or more threads attempt to load().
And this is the correct way, IMO.
With the old version the first will start to load() and all subsequent
calls to getObject() would immediately return "null" believing the model
object is already loaded.
I'm not sure how valid this problem really is.
Wicket guarantees synchronization per page instance. Everything else should
be handled by the application itself.
>
> Thanks
> Sven
>
>
>
> On 24.11.2014 23:39, Martijn Dashorst wrote:
>
>> It appears innocent enough this change, but we can't be sure this
>> won't break applications: isAttached() is a public method and it is
>> valid to call inside load(). If someone depends on isAttached() during
>> load() we will change this behavior.
>>
>> Granted this new semantics are much more to my liking, and it fixes a
>> bug. But does this justify the risk of breaking applications in other
>> ways?
>>
>> I checked some of our projects for such mishaps but couldn't find any
>> misuses of isAttached inside load().
>>
>> Martijn
>>
>>
>> On Mon, Nov 24, 2014 at 9:11 PM, Sven Meier (JIRA) <ji...@apache.org>
>> wrote:
>>
>>> [ https://issues.apache.org/jira/browse/WICKET-5772?page=
>>> com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
>>>
>>> Sven Meier resolved WICKET-5772.
>>> --------------------------------
>>> Resolution: Fixed
>>> Fix Version/s: 6.19.0
>>> 7.0.0-M5
>>>
>>> #attached is now set after calling #load(), as suggested.
>>>
>>> Thanks Martin!
>>>
>>> LoadableDetachableModel caches null value if load() fails, bug in
>>>> getObject() {attached = true;}
>>>> ------------------------------------------------------------
>>>> ------------------------------------
>>>>
>>>> Key: WICKET-5772
>>>> URL: https://issues.apache.org/jira/browse/WICKET-5772
>>>> Project: Wicket
>>>> Issue Type: Bug
>>>> Components: wicket
>>>> Affects Versions: 1.4.23, 6.18.0, 7.0.0-M4
>>>> Reporter: Martin Makundi
>>>> Assignee: Sven Meier
>>>> Labels: easyfix, easytest, patch
>>>> Fix For: 7.0.0-M5, 6.19.0
>>>>
>>>> Original Estimate: 10m
>>>> Remaining Estimate: 10m
>>>>
>>>> If you have a LoadableDetachableModel whose load() operation fails at
>>>> an instance, the LoadableDetachableModel will cache null value because
>>>> getObject() method sets attached = true; before load() invocation has
>>>> returned.
>>>> This results in methods trusting LoadableDetachableModel using the null
>>>> value for their operations which is logically incorrect as null might not
>>>> be a legal value at all. Such behavior would result in unexpected
>>>> difficult-to-debug behavior in depending components.
>>>> Easy fix:
>>>> Move:
>>>> attached = true;
>>>> after line:
>>>> transientModelObject = load();
>>>> Test case:
>>>> /**
>>>> * LoadableDetachableModel must not return null as an attached value
>>>> if load()
>>>> * fails.
>>>> */
>>>> public void testWhenLoadFails() {
>>>> final LoadableDetachableModel<Void> loadableDetachableModel = new
>>>> LoadableDetachableModel<Void>() {
>>>> /**
>>>> * @see org.apache.wicket.model.LoadableDetachableModel#load()
>>>> */
>>>> @Override
>>>> protected Void load() {
>>>> throw new UnsupportedOperationException("Let's assume this
>>>> fails for some reason.");
>>>> }
>>>> };
>>>> try {
>>>> loadableDetachableModel.getObject();
>>>> fail(UnsupportedOperationException.class + " expected.");
>>>> } catch (final UnsupportedOperationException e) {
>>>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected
>>>> behavior due to " + UnsupportedOperationException.class);
>>>> }
>>>> try {
>>>> assertNotSame(LoadableDetachableModel.class + " should not
>>>> return null if it's load() has failed\n", null,
>>>> loadableDetachableModel.getObject());
>>>> } catch (final UnsupportedOperationException e) {
>>>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected
>>>> behavior due to " + UnsupportedOperationException.class);
>>>> }
>>>> }
>>>>
>>>
>>>
>>> --
>>> This message was sent by Atlassian JIRA
>>> (v6.3.4#6332)
>>>
>>
>>
>>
>
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Sven Meier <sv...@meiers.net>.
+0 on reverting the change.
LDM worked fine for all those years, we don't need the change on 6.x.
Sven
On 25.11.2014 20:44, Martin Grigorov wrote:
> Then let's revert the change and add javadoc explaining that if load()
> fails and you want to re-try then you should manually detach the model
> before that ?!
>
> Martin Grigorov
> Wicket Training and Consulting
> https://twitter.com/mtgrigorov
>
> On Tue, Nov 25, 2014 at 1:34 PM, Martijn Dashorst <
> martijn.dashorst@gmail.com> wrote:
>
>> A common thing to do is to add more models to one model:
>>
>> class FooModel extends LDM<Foo> {
>> private IModel<Bar> barModel;
>> @Inject private FooService fooService;
>>
>> public FooModel(IModel<Bar> barModel) {
>> this.barModel = barModel;
>> NonContextual.of(FooModel.class).get().inject(this);
>> }
>>
>> @Override public Foo load() {
>> SomeValue value = barModel.getObject().getSomeValue();
>> return fooService.findBySomeValue(value);
>> }
>> @Override public void onDetach() {
>> barModel.detach();
>> }
>> }
>>
>> Attachment/detachment of both FooModel and BarModel are handled by the
>> FooModel.
>>
>> The issue is that if FooModel doesn't get detached anymore, BarModel
>> will remain attached as well.
>>
>> Martijn
>>
>>
>>
>> On Tue, Nov 25, 2014 at 10:01 AM, Martin Grigorov <mg...@apache.org>
>> wrote:
>>> On Tue, Nov 25, 2014 at 2:01 AM, Martijn Dashorst <
>>> martijn.dashorst@gmail.com> wrote:
>>>
>>>> On Tue, Nov 25, 2014 at 12:11 AM, Sven Meier <sv...@meiers.net> wrote:
>>>>> Hi Martin,
>>>>>
>>>>>> isAttached() is a public method and it is valid to call inside load()
>>>>> that's a valid point to consider: But what purpose could it have to
>> call
>>>>> isAttached() in load()?
>>>> Probably none, I'm playing devil's advocate here. It is one model we
>>>> promote heavily to our users so I imagine it being quite popular. What
>>>> could break if we move the attached = true; and people rely on it to
>>>> be true at the beginning of load()?
>>>>
>>> I also don't see a use case for relying on attached being true during
>>> load().
>>>
>>>
>>>> But load() is probably not the problem here... detach is...
>>>>
>>>>
>>>> /**
>>>> * @see org.apache.wicket.model.IDetachable#detach()
>>>> */
>>>> @Override
>>>> public void detach()
>>>> {
>>>> if (attached)
>>>> {
>>>> try
>>>> {
>>>> onDetach();
>>>> }
>>>> finally
>>>> {
>>>> attached = false;
>>>> transientModelObject = null;
>>>>
>>>> log.debug("removed transient object for {}, requestCycle
>> {}",
>>>> this,
>>>> RequestCycle.get());
>>>> }
>>>> }
>>>> }
>>>>
>>>> now that attached = true comes after the call to load, the model is no
>>>> longer attached. This means that any state built up during load() will
>>>> no longer be detached (removed) when something goes awry during
>>>> load().
>>>>
>>> Usually models are used to get the underlying model object, i.e.
>>> myLDM.getObject().
>>> So I don't expect an application to do something like:
>>> MyLDM myLDM = ....
>>> SomethingElse sthgElse = myLDM.getSomethingElse();
>>>
>>> If sthgElse depends on load() then the only way to guarantee it is
>> properly
>>> initialized is to use #isAttached() in #getSomethingElse().
>>> With the new improvement this should be even better than before.
>>>
>>> We can add explanation to load()'s javadoc that the application should
>>> clean up anything custom stored in the LDM itself
>>>
>>>
>>>> Martijn
>>>>
>>
>>
>> --
>> Become a Wicket expert, learn from the best: http://wicketinaction.com
>>
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Martin Grigorov <mg...@apache.org>.
Then let's revert the change and add javadoc explaining that if load()
fails and you want to re-try then you should manually detach the model
before that ?!
Martin Grigorov
Wicket Training and Consulting
https://twitter.com/mtgrigorov
On Tue, Nov 25, 2014 at 1:34 PM, Martijn Dashorst <
martijn.dashorst@gmail.com> wrote:
> A common thing to do is to add more models to one model:
>
> class FooModel extends LDM<Foo> {
> private IModel<Bar> barModel;
> @Inject private FooService fooService;
>
> public FooModel(IModel<Bar> barModel) {
> this.barModel = barModel;
> NonContextual.of(FooModel.class).get().inject(this);
> }
>
> @Override public Foo load() {
> SomeValue value = barModel.getObject().getSomeValue();
> return fooService.findBySomeValue(value);
> }
> @Override public void onDetach() {
> barModel.detach();
> }
> }
>
> Attachment/detachment of both FooModel and BarModel are handled by the
> FooModel.
>
> The issue is that if FooModel doesn't get detached anymore, BarModel
> will remain attached as well.
>
> Martijn
>
>
>
> On Tue, Nov 25, 2014 at 10:01 AM, Martin Grigorov <mg...@apache.org>
> wrote:
> > On Tue, Nov 25, 2014 at 2:01 AM, Martijn Dashorst <
> > martijn.dashorst@gmail.com> wrote:
> >
> >> On Tue, Nov 25, 2014 at 12:11 AM, Sven Meier <sv...@meiers.net> wrote:
> >> > Hi Martin,
> >> >
> >> >> isAttached() is a public method and it is valid to call inside load()
> >> >
> >> > that's a valid point to consider: But what purpose could it have to
> call
> >> > isAttached() in load()?
> >>
> >> Probably none, I'm playing devil's advocate here. It is one model we
> >> promote heavily to our users so I imagine it being quite popular. What
> >> could break if we move the attached = true; and people rely on it to
> >> be true at the beginning of load()?
> >>
> >
> > I also don't see a use case for relying on attached being true during
> > load().
> >
> >
> >>
> >> But load() is probably not the problem here... detach is...
> >>
> >>
> >> /**
> >> * @see org.apache.wicket.model.IDetachable#detach()
> >> */
> >> @Override
> >> public void detach()
> >> {
> >> if (attached)
> >> {
> >> try
> >> {
> >> onDetach();
> >> }
> >> finally
> >> {
> >> attached = false;
> >> transientModelObject = null;
> >>
> >> log.debug("removed transient object for {}, requestCycle
> {}",
> >> this,
> >> RequestCycle.get());
> >> }
> >> }
> >> }
> >>
> >> now that attached = true comes after the call to load, the model is no
> >> longer attached. This means that any state built up during load() will
> >> no longer be detached (removed) when something goes awry during
> >> load().
> >>
> >
> > Usually models are used to get the underlying model object, i.e.
> > myLDM.getObject().
> > So I don't expect an application to do something like:
> > MyLDM myLDM = ....
> > SomethingElse sthgElse = myLDM.getSomethingElse();
> >
> > If sthgElse depends on load() then the only way to guarantee it is
> properly
> > initialized is to use #isAttached() in #getSomethingElse().
> > With the new improvement this should be even better than before.
> >
> > We can add explanation to load()'s javadoc that the application should
> > clean up anything custom stored in the LDM itself
> >
> >
> >>
> >> Martijn
> >>
>
>
>
> --
> Become a Wicket expert, learn from the best: http://wicketinaction.com
>
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Martijn Dashorst <ma...@gmail.com>.
A common thing to do is to add more models to one model:
class FooModel extends LDM<Foo> {
private IModel<Bar> barModel;
@Inject private FooService fooService;
public FooModel(IModel<Bar> barModel) {
this.barModel = barModel;
NonContextual.of(FooModel.class).get().inject(this);
}
@Override public Foo load() {
SomeValue value = barModel.getObject().getSomeValue();
return fooService.findBySomeValue(value);
}
@Override public void onDetach() {
barModel.detach();
}
}
Attachment/detachment of both FooModel and BarModel are handled by the FooModel.
The issue is that if FooModel doesn't get detached anymore, BarModel
will remain attached as well.
Martijn
On Tue, Nov 25, 2014 at 10:01 AM, Martin Grigorov <mg...@apache.org> wrote:
> On Tue, Nov 25, 2014 at 2:01 AM, Martijn Dashorst <
> martijn.dashorst@gmail.com> wrote:
>
>> On Tue, Nov 25, 2014 at 12:11 AM, Sven Meier <sv...@meiers.net> wrote:
>> > Hi Martin,
>> >
>> >> isAttached() is a public method and it is valid to call inside load()
>> >
>> > that's a valid point to consider: But what purpose could it have to call
>> > isAttached() in load()?
>>
>> Probably none, I'm playing devil's advocate here. It is one model we
>> promote heavily to our users so I imagine it being quite popular. What
>> could break if we move the attached = true; and people rely on it to
>> be true at the beginning of load()?
>>
>
> I also don't see a use case for relying on attached being true during
> load().
>
>
>>
>> But load() is probably not the problem here... detach is...
>>
>>
>> /**
>> * @see org.apache.wicket.model.IDetachable#detach()
>> */
>> @Override
>> public void detach()
>> {
>> if (attached)
>> {
>> try
>> {
>> onDetach();
>> }
>> finally
>> {
>> attached = false;
>> transientModelObject = null;
>>
>> log.debug("removed transient object for {}, requestCycle {}",
>> this,
>> RequestCycle.get());
>> }
>> }
>> }
>>
>> now that attached = true comes after the call to load, the model is no
>> longer attached. This means that any state built up during load() will
>> no longer be detached (removed) when something goes awry during
>> load().
>>
>
> Usually models are used to get the underlying model object, i.e.
> myLDM.getObject().
> So I don't expect an application to do something like:
> MyLDM myLDM = ....
> SomethingElse sthgElse = myLDM.getSomethingElse();
>
> If sthgElse depends on load() then the only way to guarantee it is properly
> initialized is to use #isAttached() in #getSomethingElse().
> With the new improvement this should be even better than before.
>
> We can add explanation to load()'s javadoc that the application should
> clean up anything custom stored in the LDM itself
>
>
>>
>> Martijn
>>
--
Become a Wicket expert, learn from the best: http://wicketinaction.com
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Martin Grigorov <mg...@apache.org>.
On Tue, Nov 25, 2014 at 2:01 AM, Martijn Dashorst <
martijn.dashorst@gmail.com> wrote:
> On Tue, Nov 25, 2014 at 12:11 AM, Sven Meier <sv...@meiers.net> wrote:
> > Hi Martin,
> >
> >> isAttached() is a public method and it is valid to call inside load()
> >
> > that's a valid point to consider: But what purpose could it have to call
> > isAttached() in load()?
>
> Probably none, I'm playing devil's advocate here. It is one model we
> promote heavily to our users so I imagine it being quite popular. What
> could break if we move the attached = true; and people rely on it to
> be true at the beginning of load()?
>
I also don't see a use case for relying on attached being true during
load().
>
> But load() is probably not the problem here... detach is...
>
>
> /**
> * @see org.apache.wicket.model.IDetachable#detach()
> */
> @Override
> public void detach()
> {
> if (attached)
> {
> try
> {
> onDetach();
> }
> finally
> {
> attached = false;
> transientModelObject = null;
>
> log.debug("removed transient object for {}, requestCycle {}",
> this,
> RequestCycle.get());
> }
> }
> }
>
> now that attached = true comes after the call to load, the model is no
> longer attached. This means that any state built up during load() will
> no longer be detached (removed) when something goes awry during
> load().
>
Usually models are used to get the underlying model object, i.e.
myLDM.getObject().
So I don't expect an application to do something like:
MyLDM myLDM = ....
SomethingElse sthgElse = myLDM.getSomethingElse();
If sthgElse depends on load() then the only way to guarantee it is properly
initialized is to use #isAttached() in #getSomethingElse().
With the new improvement this should be even better than before.
We can add explanation to load()'s javadoc that the application should
clean up anything custom stored in the LDM itself
>
> Martijn
>
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Martijn Dashorst <ma...@gmail.com>.
On Tue, Nov 25, 2014 at 12:11 AM, Sven Meier <sv...@meiers.net> wrote:
> Hi Martin,
>
>> isAttached() is a public method and it is valid to call inside load()
>
> that's a valid point to consider: But what purpose could it have to call
> isAttached() in load()?
Probably none, I'm playing devil's advocate here. It is one model we
promote heavily to our users so I imagine it being quite popular. What
could break if we move the attached = true; and people rely on it to
be true at the beginning of load()?
But load() is probably not the problem here... detach is...
/**
* @see org.apache.wicket.model.IDetachable#detach()
*/
@Override
public void detach()
{
if (attached)
{
try
{
onDetach();
}
finally
{
attached = false;
transientModelObject = null;
log.debug("removed transient object for {}, requestCycle {}", this,
RequestCycle.get());
}
}
}
now that attached = true comes after the call to load, the model is no
longer attached. This means that any state built up during load() will
no longer be detached (removed) when something goes awry during
load().
Martijn
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
Posted by Sven Meier <sv...@meiers.net>.
Hi Martin,
> isAttached() is a public method and it is valid to call inside load()
that's a valid point to consider: But what purpose could it have to call
isAttached() in load()?
It would return <true> during the complete invocation.
Since Wicket allows a single thread only to access the component tree, I
don't see a race condition either.
Thanks
Sven
On 24.11.2014 23:39, Martijn Dashorst wrote:
> It appears innocent enough this change, but we can't be sure this
> won't break applications: isAttached() is a public method and it is
> valid to call inside load(). If someone depends on isAttached() during
> load() we will change this behavior.
>
> Granted this new semantics are much more to my liking, and it fixes a
> bug. But does this justify the risk of breaking applications in other
> ways?
>
> I checked some of our projects for such mishaps but couldn't find any
> misuses of isAttached inside load().
>
> Martijn
>
>
> On Mon, Nov 24, 2014 at 9:11 PM, Sven Meier (JIRA) <ji...@apache.org> wrote:
>> [ https://issues.apache.org/jira/browse/WICKET-5772?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
>>
>> Sven Meier resolved WICKET-5772.
>> --------------------------------
>> Resolution: Fixed
>> Fix Version/s: 6.19.0
>> 7.0.0-M5
>>
>> #attached is now set after calling #load(), as suggested.
>>
>> Thanks Martin!
>>
>>> LoadableDetachableModel caches null value if load() fails, bug in getObject() {attached = true;}
>>> ------------------------------------------------------------------------------------------------
>>>
>>> Key: WICKET-5772
>>> URL: https://issues.apache.org/jira/browse/WICKET-5772
>>> Project: Wicket
>>> Issue Type: Bug
>>> Components: wicket
>>> Affects Versions: 1.4.23, 6.18.0, 7.0.0-M4
>>> Reporter: Martin Makundi
>>> Assignee: Sven Meier
>>> Labels: easyfix, easytest, patch
>>> Fix For: 7.0.0-M5, 6.19.0
>>>
>>> Original Estimate: 10m
>>> Remaining Estimate: 10m
>>>
>>> If you have a LoadableDetachableModel whose load() operation fails at an instance, the LoadableDetachableModel will cache null value because
>>> getObject() method sets attached = true; before load() invocation has returned.
>>> This results in methods trusting LoadableDetachableModel using the null value for their operations which is logically incorrect as null might not be a legal value at all. Such behavior would result in unexpected difficult-to-debug behavior in depending components.
>>> Easy fix:
>>> Move:
>>> attached = true;
>>> after line:
>>> transientModelObject = load();
>>> Test case:
>>> /**
>>> * LoadableDetachableModel must not return null as an attached value if load()
>>> * fails.
>>> */
>>> public void testWhenLoadFails() {
>>> final LoadableDetachableModel<Void> loadableDetachableModel = new LoadableDetachableModel<Void>() {
>>> /**
>>> * @see org.apache.wicket.model.LoadableDetachableModel#load()
>>> */
>>> @Override
>>> protected Void load() {
>>> throw new UnsupportedOperationException("Let's assume this fails for some reason.");
>>> }
>>> };
>>> try {
>>> loadableDetachableModel.getObject();
>>> fail(UnsupportedOperationException.class + " expected.");
>>> } catch (final UnsupportedOperationException e) {
>>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected behavior due to " + UnsupportedOperationException.class);
>>> }
>>> try {
>>> assertNotSame(LoadableDetachableModel.class + " should not return null if it's load() has failed\n", null,
>>> loadableDetachableModel.getObject());
>>> } catch (final UnsupportedOperationException e) {
>>> LoggerFactory.getLogger(LoadableDetachableModel.class).debug("Expected behavior due to " + UnsupportedOperationException.class);
>>> }
>>> }
>>
>>
>> --
>> This message was sent by Atlassian JIRA
>> (v6.3.4#6332)
>
>