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

Updated ThrottledBackgroundMessageProcessor to dynamically create task workers #542

Merged
merged 9 commits into from
Aug 25, 2024

Conversation

phillip-haydon
Copy link
Contributor

Should resolve #541 where up to 8 tasks workers can sit idle doing nothing when there's no messages to be sent. This change will create the task workers on demand based on the queue length and dispose of them if idle.

Copy link
Contributor

@QuantumNightmare QuantumNightmare left a comment

Choose a reason for hiding this comment

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

Took a bit to follow this but looks like it'll do the trick. Cancelling a worker causes it to complete its current task before disposing right? Make sure to do a test of spamming a fixed number of exception reports and confirming that the correct number of exceptions show up in Raygun. Maybe force exceptions to occur at certain points of the worker processing and check that it behaves as expected as well.

Copy link
Contributor

@MattByers MattByers left a comment

Choose a reason for hiding this comment

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

I can't see any immediately obvious issues. Same as Jase mentioned though, I am curious about the behaviour that happens, when a task that is currently doing work is cancelled. Does it definitely complete it's current message?

Another thought. This class looks to be duplicated at Mindscape.Raygun4Net4.ThrottledBackgroundMessageProcessor. Is it worth carrying these changes over there, so it helps with the idle tasks for .net framework.

@phillip-haydon
Copy link
Contributor Author

Took a bit to follow this but looks like it'll do the trick. Cancelling a worker causes it to complete its current task before disposing right? Make sure to do a test of spamming a fixed number of exception reports and confirming that the correct number of exceptions show up in Raygun. Maybe force exceptions to occur at certain points of the worker processing and check that it behaves as expected as well.

I ran a benchmark with 100k messages and found a race condition during the draining which caused ~1-5 messages to get stuck and not processed. This happened because when the AdjustWorkers is run, it grabs a count and once a count is obtained, but before the adjustment occurs, a message could come in and be queued, but we only allow 1 thread to enter the method at a time.

Solution was to just double-check the queue size before exiting the AjustWorkers and the issue goes away.

Besides this it appears to work well. In all cases the queue gets processed quicker than the old implementation, but somewhere around ~1k messages in the queue the memory usage grows a bit more than the old implementation, I think this is due to the Concurrent Queue resizing internally. But at 100k messages the difference looks to be ~5mb difference (45mb vs 50mb)

This gets cleaned up tho when the queue is processed.

In the old implementation we used a BlockingCollection which has a capacity argument, the ConcurrentQueue doesn't have this, but I have a check in the Queue with a drain percentage so if we hit the capacity it will drain 10% before accepting anymore messages. Give it some breathing room.

@phillip-haydon
Copy link
Contributor Author

Another thought. This class looks to be duplicated at Mindscape.Raygun4Net4.ThrottledBackgroundMessageProcessor. Is it worth carrying these changes over there, so it helps with the idle tasks for .net framework.

I only want to deal with the .NET Core version for now, may consider back-porting it to .NET Framework in the future.

ProRedCat
ProRedCat previously approved these changes Aug 22, 2024
Copy link
Contributor

@ProRedCat ProRedCat left a comment

Choose a reason for hiding this comment

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

Lgtm, this appears to have fixed some issues in Raygun4Maui too

…e and remove from cache as it cannot be delivered
@phillip-haydon phillip-haydon merged commit b9c7331 into master Aug 25, 2024
1 check passed
@phillip-haydon phillip-haydon deleted the ph/idle-tasks branch August 25, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thread pool exhaustion on version 11 of Mindscape.Raygun4Net.AspNetCore
4 participants