sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Martin Desruisseaux (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (SIS-156) @ThreadSafe and @Immutable annotation usages are misleading
Date Thu, 26 Dec 2013 02:53:50 GMT

     [ https://issues.apache.org/jira/browse/SIS-156?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Martin Desruisseaux updated SIS-156:
------------------------------------

    Description: 
Many SIS classes having {{@ThreadSafe}} or {{@Immutable}} annotation are only tainted: the
annotation applies only in a shallow sense. For example {{ImmutableIdentifier}} is truly immutable
only if the {{Citation}} given to the constructor is also immutable - this citation is not
cloned, since doing so would be potentially very costly (metadata tree can be depth). Furthermore,
we can not force subclasses to honour the immutability contract.

The {{@ThreadSafe}} and {{@Immutable}} annotations are strongly inspired from the _Java concurrency
in practice_ book, which uses a stricter interpretation of immutability: an immutable class
can reference only immutable objects (or primitive types), or clone them. A search on internet
suggests that most other libraries having such annotations interpret them in the stricter
sense.

The majority of SIS classes do not conform to the strict interpretation of immutability. This
is because many objects are aggregations of other objects for which the type is an interface.
We can not impose immutability or thread-safety constraints on implementations of interfaces.
Consequently for the majority of SIS classes, the thread-safety behaviour is more complicated
than a binary "thread-safe / not thread safe" or "immutable / mutable" binary choice.

The proposal is to simply drop the {{@ThreadSafe}} and {{@Immutable}} annotations from SIS,
and replace them by a paragraph in class javadoc.

h3. Javadoc examples
A typical sentence is "_The same {{Foo}} instance can be safely used by many threads without
synchronization on the part of the caller.  Subclasses should make sure that any overridden
methods remain safe to call from multiple threads_". However the following lists some examples
of documentation which are difficult to catch with a simple {{@ThreadSafe}} annotation:

* This class is thread-safe if and only if the {{Set}} given to the constructor is thread-safe.
* This class is immutable, and thus inherently thread-safe, if the converter given to the
constructor is also immutable.
* This base class is safe for multi-threads usage. Subclasses registered in {{META-INF/services/}}
shall be thread-safe too.
* Instances of {{DefaultInternationalString}} are thread-safe. While those instances are not
strictly immutable, SIS typically references them as if they were immutable because of their
_add-only_ behaviour.
* This class is immutable and thus inherently thread-safe if the {{Citation}} and {{InternationalString}}
arguments given to the constructor are also immutable. It is caller's responsibility to ensure
that those conditions hold, for example by invoking {{DefaultCitation.freeze()}} before passing
the arguments to the constructor. Subclasses shall make sure that any overridden methods remain
safe to call from multiple threads and do not change any public {{ImmutableIdentifier}} state.
* This base class is immutable if the {{Citation}}, {{ReferenceIdentifier}}, {{GenericName}}
and {{InternationalString}} instances given to the constructor are also immutable. Most SIS
subclasses and related classes are immutable under similar conditions. This means that unless
otherwise noted in the javadoc, {{IdentifiedObject}} instances created using only SIS factories
and static constants can be shared by many objects and passed between threads without synchronization.
* This class and the {{Latitude}} / {{Longitude}} subclasses are immutable, and thus inherently
thread-safe. Other subclasses may or may not be immutable, at implementation choice (see {{java.lang.Number}}
for an example of a similar in purpose class having mutable subclasses).


  was:
Many SIS classes having {{@ThreadSafe}} or {{@Immutable}} annotation are only tainted: the
annotation applies only in a shallow sense. For example {{ImmutableIdentifier}} is truly immutable
only if the {{Citation}} given to the constructor is also immutable - this citation is not
cloned, since doing so would be potentially very costly (metadata tree can be depth). Furthermore,
we can not force subclasses to honour the immutability contract.

The {{@ThreadSafe}} and {{@Immutable}} annotations are strongly inspired from the _Java concurrency
in practice_ book, which uses a stricter interpretation of immutability: an immutable class
can reference only immutable objects (or primitive types), or clone them. A search on internet
suggests that most other libraries having such annotations interpret them in the stricter
sense.

The majority of SIS classes do not conform to the strict interpretation of immutability. This
is because many objects are aggregations of other objects for which the type is an interface.
We can not impose immutability or thread-safety constraints on implementations of interfaces.
Consequently for the majority of SIS classes, the thread-safety behaviour is more complicated
than a binary "thread-safe / not thread safe" or "immutable / mutable" binary choice.

The proposal is to simply drop the {{@ThreadSafe}} and {{@Immutable}} annotations from SIS,
and replace them by a paragraph in class javadoc.

h3. Javadoc examples
A typical sentence is "_The same {{Foo}} instance can be safely used by many threads without
synchronization on the part of the caller.  Subclasses should make sure that any overridden
methods remain safe to call from multiple threads_". However the following lists some examples
of documentation which are difficult to catch with a simple {{@ThreadSafe}} annotation:

* This class is thread-safe if and only if the {{Set}} given to the constructor is thread-safe.
* This base class is safe for multi-threads usage. Subclasses registered in {{META-INF/services/}}
shall be thread-safe too.
* Instances of {{DefaultInternationalString}} are thread-safe. While those instances are not
strictly immutable, SIS typically references them as if they were immutable because of their
_add-only_ behaviour.
* This base class is immutable if the {{Citation}}, {{ReferenceIdentifier}}, {{GenericName}}
and {{InternationalString}} instances given to the constructor are also immutable. Most SIS
subclasses are immutable under the same conditions.



> @ThreadSafe and @Immutable annotation usages are misleading
> -----------------------------------------------------------
>
>                 Key: SIS-156
>                 URL: https://issues.apache.org/jira/browse/SIS-156
>             Project: Spatial Information Systems
>          Issue Type: Bug
>          Components: Metadata, Referencing, Utilities
>    Affects Versions: 0.3
>            Reporter: Martin Desruisseaux
>            Assignee: Martin Desruisseaux
>            Priority: Minor
>             Fix For: 0.4
>
>
> Many SIS classes having {{@ThreadSafe}} or {{@Immutable}} annotation are only tainted:
the annotation applies only in a shallow sense. For example {{ImmutableIdentifier}} is truly
immutable only if the {{Citation}} given to the constructor is also immutable - this citation
is not cloned, since doing so would be potentially very costly (metadata tree can be depth).
Furthermore, we can not force subclasses to honour the immutability contract.
> The {{@ThreadSafe}} and {{@Immutable}} annotations are strongly inspired from the _Java
concurrency in practice_ book, which uses a stricter interpretation of immutability: an immutable
class can reference only immutable objects (or primitive types), or clone them. A search on
internet suggests that most other libraries having such annotations interpret them in the
stricter sense.
> The majority of SIS classes do not conform to the strict interpretation of immutability.
This is because many objects are aggregations of other objects for which the type is an interface.
We can not impose immutability or thread-safety constraints on implementations of interfaces.
Consequently for the majority of SIS classes, the thread-safety behaviour is more complicated
than a binary "thread-safe / not thread safe" or "immutable / mutable" binary choice.
> The proposal is to simply drop the {{@ThreadSafe}} and {{@Immutable}} annotations from
SIS, and replace them by a paragraph in class javadoc.
> h3. Javadoc examples
> A typical sentence is "_The same {{Foo}} instance can be safely used by many threads
without synchronization on the part of the caller.  Subclasses should make sure that any overridden
methods remain safe to call from multiple threads_". However the following lists some examples
of documentation which are difficult to catch with a simple {{@ThreadSafe}} annotation:
> * This class is thread-safe if and only if the {{Set}} given to the constructor is thread-safe.
> * This class is immutable, and thus inherently thread-safe, if the converter given to
the constructor is also immutable.
> * This base class is safe for multi-threads usage. Subclasses registered in {{META-INF/services/}}
shall be thread-safe too.
> * Instances of {{DefaultInternationalString}} are thread-safe. While those instances
are not strictly immutable, SIS typically references them as if they were immutable because
of their _add-only_ behaviour.
> * This class is immutable and thus inherently thread-safe if the {{Citation}} and {{InternationalString}}
arguments given to the constructor are also immutable. It is caller's responsibility to ensure
that those conditions hold, for example by invoking {{DefaultCitation.freeze()}} before passing
the arguments to the constructor. Subclasses shall make sure that any overridden methods remain
safe to call from multiple threads and do not change any public {{ImmutableIdentifier}} state.
> * This base class is immutable if the {{Citation}}, {{ReferenceIdentifier}}, {{GenericName}}
and {{InternationalString}} instances given to the constructor are also immutable. Most SIS
subclasses and related classes are immutable under similar conditions. This means that unless
otherwise noted in the javadoc, {{IdentifiedObject}} instances created using only SIS factories
and static constants can be shared by many objects and passed between threads without synchronization.
> * This class and the {{Latitude}} / {{Longitude}} subclasses are immutable, and thus
inherently thread-safe. Other subclasses may or may not be immutable, at implementation choice
(see {{java.lang.Number}} for an example of a similar in purpose class having mutable subclasses).



--
This message was sent by Atlassian JIRA
(v6.1.5#6160)

Mime
View raw message