-
Notifications
You must be signed in to change notification settings - Fork 91
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
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. |
I only want to deal with the .NET Core version for now, may consider back-porting it to .NET Framework in the future. |
Mindscape.Raygun4Net.NetCore.Tests/ThrottledBackgroundMessageProcessorTests.cs
Outdated
Show resolved
Hide resolved
Mindscape.Raygun4Net.NetCore.Tests/ThrottledBackgroundMessageProcessorTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
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.