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 2015/05/29 18:27:21 UTC
Re: [jira] [Resolved] (WICKET-5772) LoadableDetachableModel caches
null value if load() fails, bug in getObject() {attached = true;}
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 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