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

Implement OnIdle engine events #1581

Merged
merged 4 commits into from
Oct 13, 2021

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Oct 12, 2021

Resolves #1433

@ghost ghost added Area-Engine Issue-Bug A bug to squash. labels Oct 12, 2021
// We didn't end up executinng anything in the background,
// so we need to run a small artificial pipeline instead
// to force event processing
if (runPipelineForEventProcessing)
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be worth explaining why, architecturally, we have to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it from here, but @daxian-dbw might know more

@@ -36,7 +36,7 @@ public BlockingConcurrentDeque()
};
}

public int Count => _queues[0].Count + _queues[1].Count;
public bool IsEmpty => _queues[0].Count == 0 && _queues[1].Count == 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI BlockingCollection has no IsEmpty property

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, gotcha.

Copy link
Member

Choose a reason for hiding this comment

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

Technically there may be a race here, no?

Copy link
Member

Choose a reason for hiding this comment

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

If any method in BlockingCollection is executing while the Count property is being accessed, the return value is approximate. Count may reflect a number that is either greater than or less than the actual number of items in the BlockingCollection.

Yup. Do we want to lock around this somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think it's worth it; we only want an approximation ourselves, and there's no point in us trying to make a hard answer out of two soft values.

Copy link
Member

Choose a reason for hiding this comment

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

The scenario I'm thinking of is where we choose to end the iteration across the items of the pipeline. There's a race between checking that the pipeline is empty and ending the iteration: a task could get queued in-between and then it would be lost, no?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the answer is going to come from the question: would it be lost, or would it simply be delayed until something else iterates over it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a task could get queued in-between and then it would be lost, no?

No, the task wouldn't be lost; we don't modify the queue if we think it's empty.

We make a decision about whether to process the queue early on based on whether there's anything in it. If the queue looks empty, nothing is processed. If we're wrong, the only thing that happens is the queued task won't be processed until the next time the queue is serviced, which will mostly be in 300ms.

As you say, if we're wrong, processing is just delayed.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Engine Issue-Bug A bug to squash.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants