Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added stop committee runner queue consumption trigger #1436

Closed
wants to merge 19 commits into from

Conversation

AKorpusenko
Copy link
Contributor

@AKorpusenko AKorpusenko commented Jun 19, 2024

No description provided.

Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

create a context when you create the queue and pass it in.

@AKorpusenko AKorpusenko requested a review from y0sher June 24, 2024 14:45
Comment on lines 81 to 90
var q queueContainer

// in case of any error try to call the ctx.cancel to prevent the ctx leak
defer func() {
var ok bool

v.mtx.Lock()
q, ok = v.Queues[slot]
v.mtx.Unlock()

Copy link
Contributor

Choose a reason for hiding this comment

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

StartDuty is already locking, so this is potential "deadlock" or just double locking, although this happens in a goroutine and the lock is released in start duty. Still I think its better to just pass the Queue q from StartDuty as we're already using the mutex to get it there.

by passing it to this function you don't need to use the mutex here at all.

Comment on lines 867 to 869
for _, slotQueue := range vc.Queues {
slotQueue.StopQueueF()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can't do this here, accessing Queues requires mutex acquiring. best is to have a function inside the committee struct that handles this.


if err := dutyRunner.GetBaseRunner().QBFTController.OnTimeout(logger, *eventMsg); err != nil {
return fmt.Errorf("timeout event: %w", err)
}

c.Queues[slot].StopQueueF()
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to lock the mutex to access Queues. the best will be to get it same time you're getting the runner at the beginning of function.

Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

concurrency issues..

@AKorpusenko AKorpusenko requested a review from y0sher June 25, 2024 13:38
Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

small change

@AKorpusenko AKorpusenko requested a review from y0sher June 25, 2024 14:36
Co-authored-by: Matus Kysel <[email protected]>
@y0sher y0sher changed the base branch from alan/no-fork to stage July 28, 2024 15:39
err := c.ConsumeQueue(logger, duty.Slot, c.ProcessMessage, r)
// Setting the cancel function separately due the queue could be created in HandleMessage

logger = c.logger.With(fields.DutyID(fields.FormatCommitteeDutyID(c.Operator.Committee, c.BeaconNetwork.EstimatedEpochAtSlot(duty.Slot), duty.Slot)), fields.Slot(duty.Slot))
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be in the other PR for committee ID no?

if err := dutyRunner.GetBaseRunner().QBFTController.OnTimeout(logger, *eventMsg); err != nil {
return fmt.Errorf("timeout event: %w", err)
}

q.StopQueueF()
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we stopping the queue on timeout message?

@AKorpusenko
Copy link
Contributor Author

Replaced with PR #1556 based on the stage branch

@AKorpusenko AKorpusenko closed this Aug 5, 2024
@AKorpusenko AKorpusenko deleted the alan/no-fork-add-consume-queue-exit branch August 5, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants