Remove ParallelExtensionsExtras dependency, replace with SemaphoreSlim#1244
Remove ParallelExtensionsExtras dependency, replace with SemaphoreSlim#1244ispwd wants to merge 1 commit into
Conversation
|
This PR is stale because it has been open for 30 days with no activity. It will be closed in 3 days if no further activity occurs. If you wish not to mark it as stale, please leave a comment in this PR. |
oss-sentinel-ai
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #1244 — Remove ParallelExtensionsExtras dependency, replace with SemaphoreSlim
Type: Compatibility fix (7 files, +49/-52)
Assessment
This PR addresses a legitimate compatibility issue:
- Problem:
ParallelExtensionsExtraswas designed for .NET Framework only, causing warnings on .NET Core/5+ - Solution: Replace
LimitedConcurrencyLevelTaskSchedulerwith nativeSemaphoreSlim— a clean, well-supported approach - Scope: Changes are contained within the C# client module with test updates
Verdict
Good modernization change. Replacing an obsolete dependency with native .NET APIs improves maintainability and compatibility.
🤖 Automated review by oss-sentinel-ai
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Removes the obsolete ParallelExtensionsExtras dependency from the C# client and replaces LimitedConcurrencyTaskScheduler with SemaphoreSlim for concurrency control.
Findings
- [Info] The old
TaskScheduler-based approach is replaced withSemaphoreSlim.WaitAsync()+Task.Run(). This is a cleaner pattern for .NET Core / .NET 5+ and removes an unmaintained dependency. - [Info] The
ConsumeServiceconstructor now takesSemaphoreSliminstead ofTaskScheduler. Callers are updated accordingly. - [Info] Error handling is simplified: the
TaskCompletionSourcewrapper is removed in favor ofasync/await. - [Warning] Ensure the
SemaphoreSlimis initialized with the correctmaxConcurrencyvalue. The oldLimitedConcurrencyTaskSchedulerhad a specific max concurrency; verify the new semaphore matches. - [Info] The
ClientConfigchanges propagate the semaphore correctly through the client initialization chain.
Suggestions
- Consider adding a comment explaining why
SemaphoreSlimwas chosen over other concurrency control mechanisms. - Verify that the semaphore release happens in a
finallyblock to prevent leaks on exception paths.
Overall: Good modernization. Clean replacement with proper async/await patterns.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Remove obsolete ParallelExtensionsExtras dependency for .NET Core / .NET 5+ compatibility.
Brief Description
Problem:
The
ParallelExtensionsExtrasNuGet package was originally designed for .NET Framework only and lacks support for .NET Core / .NET 5+. This caused compatibility warnings and prevented the library from being used in modern .NET projects without fallback mode.Solution:
ParallelExtensionsExtrasNuGet package dependency entirelyLimitedConcurrencyLevelTaskScheduler(from ParallelExtensionsExtras) with native .NETSemaphoreSlimto implement the same concurrency control behaviorSemaphoreSlimis initialized withconsumptionThreadCountto limit concurrent message consumers, maintaining exactly the same semantic behavior as the original implementationFiles Changed:
ConsumeService.cs- Changed fromTaskSchedulertoSemaphoreSlimfor concurrency controlPushConsumer.cs- Updated field type and initializationFifoConsumeService.cs,StandardConsumeService.cs- Updated constructor signaturesConsumeServiceTest.cs- Updated tests to match new implementationrocketmq-client-csharp.csproj- RemovedParallelExtensionsExtraspackage referenceHow Did You Test This Change?
1. Unit Tests: ✅ All 164 existing unit tests passed
2. Concurrency Control Verification (Real RocketMQ Server): ✅
consumptionThreadCount = 33. Backward Compatibility: ✅
ConsumptionThreadCountconfiguration option works exactly as before