Skip to:

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

Make Component Initialization & destruction non-idempotent

Description

This came out of discussions in .

Currently the component base classes allow (and indeed encourage) initialize and destroy to be called multiple times.

The implementation tries to make the calls to doInitialize and doDestroy only happen once, but there is not protection against multi-threaded operation.

To be discussed but we probably want to

  • Stop testing for idempotency

  • Run through all our code explicitly removing the places where init or destroy is called mu;tiple times.

  • At that stage we have three choices

    • Throw an exception if we are called multiple times

    • Leave the “try to make it work” code in place

    • Try to add syncrhonization. This isn’t as easy as it sounds since there is a very real (if contrived) way that the obvious fix (add syncrhonized to the destroy method) will deadlock (Component A destroys Component B, Component B destroys A and two threads destroy Component A and Component B at the same time).

Environment

None

Activity

Show:

Rod WiddowsonAugust 22, 2022 at 1:12 PM

I think that we decided that this was a bad idea. Unless someone chips in I’ll close this as won’t do in a day or two.

Scott CantorJuly 18, 2022 at 12:18 PM

If it comes down to it, we ought to be able to set a flag that indicates whether the bean was created by Spring or not (since we’d have to be creating it some other way).

Rod WiddowsonJuly 16, 2022 at 10:54 AM

I dove into this a bit further and came upon this interesting comment (of mine) in net.shibboleth.ext.spring.service.AbstractServiceableComponent line1165 ff

I am not yet sure whether this matters or not. All services are created by Spring and it seems reasonable to assume that most other beans are as well. But are we confident that this is and will always be the case? On the other hand, what is the impact of getting this wrong? In the Service case we will leak threads and in other cases we might leak open file references.

This was destroy. I’ll look at initialize ‘some time’

Rod WiddowsonJuly 15, 2022 at 3:07 PM

As per the meeting notes I will

  • Look at the places where we are abusing this

  • Add a litmus test for the cases whgere we should be delegating to spring.

Ian YoungJuly 14, 2022 at 11:10 AM

My previous comments about wanting to make sure things don’t get called more than once of course applies mainly to the doInitialise and doDestroy chains; I’m OK with the idea that initialize and destroy are idempotent.

The implementation in AbstractInitializableComponent, as you point out, attempts to assure this.

I’d also note, though, that the implementation in AbstractInitializableComponent also already uses synchronized to try to address the thread safety issue. So at a first glance, there’s nothing to do here.

The exception is your concern about deadlock, but I’m not sure that I understand the sequence of events you’re concerned with. Remember that a single thread can hold the lock on some objects and then re-enter their synchronized methods without deadlocking; it’s just other threads that can’t do that. So if a thread is in the middle of destroying component A which involves being in the middle of destroying component A, then calling back to component A’s destroy is just fine. While in that state, moreover, a second thread will be blocked from entering either object’s synchronized methods.

So there probably is a sequence of events which would result in deadlock but you need to lay it out in detail so that we can understand whether it’s an issue. I haven’t been able to find a plausible one, but maybe I just lack imagination.

Won't Do

Details

Assignee

Reporter

Created July 14, 2022 at 10:13 AM
Updated September 4, 2022 at 12:41 PM
Resolved September 4, 2022 at 12:41 PM