opal hotel: only delete events that have not yet fired

[jsquyres]

The opal_hotel_checkout() function can be called manually or it can be invoked by the eviction callback. When it is invoked by the eviction callback, we do not want to opal_event_del() the eviction callback.

@rhc54 Please review -- I think that this is a bug in the hotel code.

[annu13]

@jsquyres - The same change is also needed in function: opal_hotel_checkout_and_return_occupant.

[jsquyres]

@annu13 Good catch -- I had that in another branch, but missed it here in the official PR. Thanks! Fixing now...

[rhc54]

@jsquyres Just to be clear: we don't want to event_del the event because libevent automatically deletes it upon callback, yes?

[jsquyres]

@rhc54 I'm 99% certain of this -- I saw weird things when I was testing some usnic stuff last night and noticed this. I couldn't fully trace it through libevent to confirm the issue, but when I added this check, it seemed to fix the issue. I had hoped you would look at this (since you're much more familiar with libevent than me) and provide a sanity check/confirmation in the form of "Oh, right! We should absolutely not be deleting the event from that event's own callback!" 😄 And also confirm that opal_event_evtimer_pending() is the Right way to check for that... 😄 😄

[rhc54]

@jsquyres I should have been clearer. libevent definitely deletes the event once the callback returns, so you should never delete an event from within a callback. I didn't realize that opal_hotel was doing so or I would have flagged it.

I'll take a closer look a little later, once I've put out some other fires.

[hppritcha]

@jsquyres this needs to go in to 2.0.0?

[rhc54]

@jsquyres I've been scratching my head over this one, and think there may be something more subtle here that needs to be clarified. Because we have an async thread that is progressing the event thread, and we have an eviction timer in that thread, you cannot just call a function and modify a room occupant because the thread can fire at any time. You can only do it from inside an event.

So if we are inside an event, then we can only delete the eviction timer if we are not in the eviction event itself. Since we have a specific callback function for eviction, you automatically know if you are in that function, and thus you know that you are in the eviction event as opposed to some other one. The only time this would be confusing is if you pointed your manual-remove event to the same function as the eviction event - and that would be a really bad design decision.

I guess my bottom-line is that I don't think the hotel class itself should be deleting any event as it just isn't safe. You need to ensure that you only remove an occupant while in an event, and you should ensure that you point the manual-remove event to a different function than the eviction event so you know what event can/should be deleted.

[jsquyres]

I think the issue here is that the hotel currently isn't async-thread safe...

In the usnic BTL use case, the hotel evbase is the synchronous thread, so it's all safe.

We should probably talk on the phone about the implications of an async thread evbase / multiple threads interacting with a hotel. We might either need a per-hotel-instance switch to enable/disable locks on a hotel, or perhaps a 2 hotel classes: one with locks, and one without...? Fodder for discussion.

[jsquyres]

@rhc54 and I talked on the phone. Current solution on this PR isn't correct: the _pending() call does not guarantee that this thread is the one that is in the callback for this event. I.e., there's no public way to know that. The simplest solution is likely to copy the relevant checkout bookkeeping code into the eviction callback glue. I'll update this PR.

[jsquyres]

@rhc54 Please review the new commit I just pushed. Thanks.

[rhc54]

👍 thx!