Skip to content

Conversation

@duke8253
Copy link
Contributor

@duke8253 duke8253 commented Feb 3, 2019

This was causing some crashes during shutdown.

@duke8253 duke8253 added the Crash label Feb 3, 2019
@duke8253 duke8253 added this to the 9.0.0 milestone Feb 3, 2019
@duke8253 duke8253 self-assigned this Feb 3, 2019
@duke8253 duke8253 requested a review from shinrich February 3, 2019 21:59
@SolidWallOfCode
Copy link
Member

Talk to @shinrich - she had the same problem and we tweaked a call (I think the mutex locking) to transparently deal with null mutexes.

Also, please work with @dyrock on plugin coordination - one of the points of that is to unify this kind of code so bugs like this don't have to stepped on one place at a time.

@shinrich
Copy link
Member

shinrich commented Feb 4, 2019

Yes, we tidied up the mutex try lock to gracefully handle the null mutex case. Need to remember where we made that change.
Functionally, this fix does address the missing lock acquisition problem.

@SolidWallOfCode
Copy link
Member

See #4926.

@duke8253 duke8253 force-pushed the master-shutdown_hook_lock branch from 1ecc844 to b6bf972 Compare February 7, 2019 16:31
APIHook *hook = lifecycle_hooks->get(TS_LIFECYCLE_SHUTDOWN_HOOK);
while (hook) {
hook->invoke(TS_EVENT_LIFECYCLE_SHUTDOWN, nullptr);
if (hook->m_cont->mutex) {
Copy link
Member

@SolidWallOfCode SolidWallOfCode Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not

while (hook) {
  SCOPED_MUTEX_LOCK(lock, hook->m_cont->mutex, this_ethread());
  hook->invoke(TS_EVENT_LIFECYCLE_SHUTDOWN, nullptr);
  hook = hook->next;
}

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the lock object will be a harmless do-nothing if the mutex is null?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It was designed that way precisely to get this effect. Better to check the null in one place, and not every callsite.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be OCD about not holding mutexes longer than necessary, I would prefer:

while (hook) {
  {
    SCOPED_MUTEX_LOCK(lock, hook->m_cont->mutex, this_ethread());
    hook->invoke(TS_EVENT_LIFECYCLE_SHUTDOWN, nullptr);
  }
  hook = hook->next;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's during shutdown, I think I want it to hold it for longer, might expose other problems in the code.

@duke8253 duke8253 force-pushed the master-shutdown_hook_lock branch from b6bf972 to 6eab7b3 Compare February 7, 2019 17:26
@duke8253 duke8253 force-pushed the master-shutdown_hook_lock branch from 6eab7b3 to 6ab78ee Compare February 7, 2019 19:46
@shinrich shinrich merged commit 72a79c0 into apache:master Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants