Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

API Review: thread safety and synchronization

Description

Use of @ThreadSafe and synchronized is somewhat sparse and inconsistent. @ThreadSafeAfterInit is never used.

I think we should document the assumption that:

  • Components are constructed and initialized on a single thread.

    • This implies that they need not be generally @ThreadSafe and there is no need to mark either constructors or property setters as synchronized.

  • Components are @ThreadSafeAfterInit

    • In some rare cases, this might require that property getters are synchronized, but I can't think of an instance where this would need to be the case in practice.

  • Components such as stages need to be reentrant: their execute method may be called simultaneously from multiple threads (e.g., in parallel pipelines).

    • This means that fields need to be either inherently @ThreadSafe, or synchronized appropriately. Synchronizing execute should be the last resort.

Environment

None

Activity

Ian YoungAugust 26, 2022 at 11:05 AM

Added some additional synchronizations suggested by SpotBugs analysis, commit ac63302e95243684d4848bc9a348a1a176110426.

Some of these (the synchronized on some doInitialize methods in particular) are not truly required, as those methods are always in practice executed while the object’s monitor is held by the initialize method in one of the parent classes. SpotBugs doesn’t know that, and I suppose in principle that constraint might be violated at some point in the future. So I decided to go ahead and synchronize those methods on the basis that synchronization while the monitor is already held is reputedly inexpensive and it means that there aren’t a lot of SpotBugs warnings I am ignoring because “I know it’s wrong about that”, which is a pretty risky approach to a tool.

In principle, it would be possible to ignore specific SpotBugs warnings on methods with specific names, but that also seems like a bad approach in general.

Ian YoungJuly 15, 2020 at 10:04 AM

Documentation done.

Ian YoungJuly 14, 2020 at 5:15 PM

Third and final tranche, commit 8af154b0e37d7857f4ae08dea5104c28ef0acee0, covers util, validate, validate.string, validate.x509.

Ian YoungJuly 14, 2020 at 5:14 PM

Second tranche, commit 8e3d404d9a6a9185da103ae881dc909e69d14851, covers dom.saml.mdattr, dom.saml.mdrpi, dom.saml.mdui, pipeline, pipeline.impl.

Ian YoungJuly 14, 2020 at 11:01 AM

First tranche of changes to the rest of the framework is in commit 288b13145b98fb958dbbdd4499137b9097b91944.

Mainly covers the root directory, dom, dom.ds, dom.impl (new package), dom.saml but also various connected classes elsewhere. Probably more than half done now.

Done

Details

Assignee

Reporter

Fix versions

Affects versions

Created May 25, 2020 at 3:21 PM
Updated May 16, 2024 at 12:37 PM
Resolved July 15, 2020 at 10:04 AM

Flag notifications