On Tue, Nov 25, 2014 at 1:11 AM, Sven Meier <[email protected]> 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) <[email protected]> >> 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) >>> >> >> >> >
