Carrot2

AttributeBinder should perform instance / class coercion only when explicitly requested in the @Attribute annotation

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 3.0
  • Fix Version/s: 3.0.1
  • Component/s: Framework Core
  • Labels:
    None

Description

Currently, CachingController remembers original attribute values once per all instances of a component (class + id pair). To make this work, we had to resort to a trick when it comes to non-primitive attributes: instead of remembering the actual instance, we remember only its class (http://fisheye3.atlassian.com/browse/carrot2/branches/3.0-proto/core/carrot2-core/src/org/carrot2/core/CachingController.java?r=3015#l534) and AttributeBinder does the class -> new instance coercion for us when resetting the values before returning the component to the pool (http://fisheye3.atlassian.com/browse/carrot2/branches/3.0-proto/core/carrot2-core/src/org/carrot2/core/CachingController.java?r=3015#l595). If we didn't do the "to class" conversion, we'd quickly mix instances of non-primitive attributes between component instances, which would result in a concurrency mess.

The problem with this approach is that this works only for classes that do not need any parameters to instantiate, and most of them indeed don't, like YahooNewsSearchService etc. But there is a class of attribute types that don't meet this requirement, e.g. ClassLoaderResource, URLResource etc. As long these are merely data object that don't maintain their own state (like the resources), we're still safe in terms of concurrency. But in a general case, this doesn't hold true.

Therefore, I was wandering – maybe we should introduce some kind of annotation on attributes that would make this instance -> class conversion explicit? Then, those attributes that indeed need the re-creation of values would use it, and for the rest we'd assume that it is safe to remember one value and populate it across other instances when resetting.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Dawid Weiss added a comment -

I haven't analyzed this issue in-depth, but from the first look it seems to me that:

1) we should not make it any more complex than it needs to be. Anything that works behind the scenes or that requires additional obscure attributes will be a pain in the long term.

2) it looks like we can overcome this with patterns already present in the spring library, can't we? I'm specifically thinking of either default constructor instantiation or a factory bean that would create objects when required. For serialization, the factory pattern could be extended to support serialization of custom types.

Whatever we decide to go with, it will render understanding of the API more difficult, so we need to be careful.

Show
Dawid Weiss added a comment - I haven't analyzed this issue in-depth, but from the first look it seems to me that: 1) we should not make it any more complex than it needs to be. Anything that works behind the scenes or that requires additional obscure attributes will be a pain in the long term. 2) it looks like we can overcome this with patterns already present in the spring library, can't we? I'm specifically thinking of either default constructor instantiation or a factory bean that would create objects when required. For serialization, the factory pattern could be extended to support serialization of custom types. Whatever we decide to go with, it will render understanding of the API more difficult, so we need to be careful.
Hide
Permalink
Stanisław Osiński added a comment -

2) it looks like we can overcome this with patterns already present in the spring library, can't we? I'm specifically thinking of either default constructor instantiation or a factory bean that would create objects when required. For serialization, the factory pattern could be extended to support serialization of custom types.

The default constructor will work for such parameters as service for YahooDataSource, but it will not for Resources, e.g. ClassLoaderResource – it doesn't make sense to use the default constructor, because you need a parameter (resource path) here (I had this problem in a real world scenario, that's the reason I filed this). When it comes to the factory, it sounds like a solution and I thought about it as well, though it will also complicate the API because we'd need an extra annotation to specify the class. Such a factory would have to have a default constructor and init-time attributes for customization.

One more option that would not require extra annotations is this:

  • if the type has an accessible default constructor – recreate the instances where required
  • if the type does not have an accessible default constructor – assume it's a data object and hence does not need to be recreated after the request has been served

The problem with it is that it hides things (just as we do now), and you may run into concurrency problems if your non-thread-safe class doesn't happen to have a default constructor.

In general the problem is non-trivial, we'll see how often it emerges in practice.

Show
Stanisław Osiński added a comment - 2) it looks like we can overcome this with patterns already present in the spring library, can't we? I'm specifically thinking of either default constructor instantiation or a factory bean that would create objects when required. For serialization, the factory pattern could be extended to support serialization of custom types. The default constructor will work for such parameters as service for YahooDataSource, but it will not for Resources, e.g. ClassLoaderResource – it doesn't make sense to use the default constructor, because you need a parameter (resource path) here (I had this problem in a real world scenario, that's the reason I filed this). When it comes to the factory, it sounds like a solution and I thought about it as well, though it will also complicate the API because we'd need an extra annotation to specify the class. Such a factory would have to have a default constructor and init-time attributes for customization. One more option that would not require extra annotations is this: if the type has an accessible default constructor – recreate the instances where required if the type does not have an accessible default constructor – assume it's a data object and hence does not need to be recreated after the request has been served The problem with it is that it hides things (just as we do now), and you may run into concurrency problems if your non-thread-safe class doesn't happen to have a default constructor. In general the problem is non-trivial, we'll see how often it emerges in practice.
Hide
Permalink
Stanisław Osiński added a comment -

Fixed by restructuring CachingController to remember / reset state for each component instance indivudually.

Show
Stanisław Osiński added a comment - Fixed by restructuring CachingController to remember / reset state for each component instance indivudually.
Hide
Permalink
Stanisław Osiński added a comment -

Closing for the 3.0.1 release.

Show
Stanisław Osiński added a comment - Closing for the 3.0.1 release.

People

  • Assignee:
    Stanisław Osiński
    Reporter:
    Stanisław Osiński
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: