Skip to content

Conversation

@knopers8
Copy link
Collaborator

Fixes a bug reported in OCTRL-999.

The recursive locking happens in the following trace:

 5  0x000000000122c829 in github.com/AliceO2Group/Control/core/task.(*Task).GetParent
    at /home/alibuild/sw/BUILD/eb488922a0fabb8512ec137d3a765b22d2941067/Control-Core/go/src/github.com/AliceO2Group/Control/core/task/task.go:741
 6  0x000000000122597c in github.com/AliceO2Group/Control/core/task.(*Task).GetTraits
    at /home/alibuild/sw/BUILD/eb488922a0fabb8512ec137d3a765b22d2941067/Control-Core/go/src/github.com/AliceO2Group/Control/core/task/task.go:248
 7  0x000000000122a217 in github.com/AliceO2Group/Control/core/task.(*Task).SendEvent
    at /home/alibuild/sw/BUILD/eb488922a0fabb8512ec137d3a765b22d2941067/Control-Core/go/src/github.com/AliceO2Group/Control/core/task/task.go:528
    ```

Fixes a bug reported in OCTRL-999.
@knopers8 knopers8 requested review from Copilot and justonedev1 March 27, 2025 16:06
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a recursive mutex locking issue in the Task implementation by bypassing the traditional lock in the GetTraits method.

  • Introduces a lock-free version of GetTraits named getTraits
  • Updates the SendEvent method to use the new lock-free version
Comments suppressed due to low confidence (2)

core/task/task.go:258

  • Please confirm that the parent's GetTaskTraits method is implemented as a lock-free or safe alternative, as calling it here may not resolve the recursive mutex locking issue if it internally acquires a lock.
return t.parent.GetTaskTraits()

core/task/task.go:255

  • [nitpick] Consider renaming 'getTraits' to more clearly indicate it is a lock-free variant (e.g., 'getTraitsLockFree') to avoid potential confusion with the original GetTraits.
func (t *Task) getTraits() Traits {

}
return Traits{}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like how it is literally the same code for GetTraits() and getTraits() except t.parent part. I'd probably make getTraits(*parentRole) and call it from GetTraits, because the t.GetParent() usage in this case is not thread safe either way. We call it twice, so if there is a change of a parent from other goroutine, we might get different results on both calls due to race condition. With this change we will at least alway address the same parent, even if it changes half during the GetTraits call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did you mean something like this?

@justonedev1 justonedev1 merged commit b865385 into AliceO2Group:master Mar 28, 2025
2 checks passed
@knopers8 knopers8 deleted the fix-octrl-999 branch March 28, 2025 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants