fix: resolve Timer.Start dueTime bugs and hardening#363
Merged
Conversation
Fix Timer.Start(TimeSpan) using .Milliseconds (component only) instead of .TotalMilliseconds, and both overloads not storing the dueTime parameter to the DueTime property. The int overload also used the stale DueTime value in the else branch instead of the parameter. Add Timer tests covering all Start overloads, stop, and dispose. Add test projects for MADE.Foundation and MADE.Runtime. Fix nullable annotations in MADE.Networking and add async safety to AppDiagnostics event handlers. Remove x64/x86 platform configurations from solution.
SummarySummary
CoverageMADE.Collections - 64.3%
MADE.Data.Converters - 81.2%
MADE.Data.EFCore - 69.1%
MADE.Data.Serialization - 82%
MADE.Data.Validation - 66.2%
MADE.Data.Validation.FluentValidation - 90%
MADE.Diagnostics - 16.1%
MADE.Foundation - 100%
MADE.Networking - 59.1%
MADE.Runtime - 59.1%
MADE.Testing - 74.4%
MADE.Threading - 74.7%
MADE.Web - 6.7%
MADE.Web.Mvc - 6%
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| { | ||
| // Arrange | ||
| var callback = CreateCallbackWithWeakTarget(); | ||
| GC.Collect(); |
| var callback = CreateCallbackWithWeakTarget(); | ||
| GC.Collect(); | ||
| GC.WaitForPendingFinalizers(); | ||
| GC.Collect(); |
| var listener = CreateListenerWithWeakInstance(out _); | ||
| listener.OnDetachAction = (_, _) => detached = true; | ||
|
|
||
| GC.Collect(); |
|
|
||
| GC.Collect(); | ||
| GC.WaitForPendingFinalizers(); | ||
| GC.Collect(); |
| catch (Exception ex) | ||
| { | ||
| successCallback.Invoke(Activator.CreateInstance(successCallback.Type)); | ||
| if (successCallback is not null) |
Comment on lines
+124
to
+127
| catch | ||
| { | ||
| // Swallow exceptions in last-resort exception handlers to prevent crashing the process. | ||
| } |
Comment on lines
+254
to
+257
| catch | ||
| { | ||
| // Swallow exceptions in timer callback to prevent unobserved task exceptions. | ||
| } |
There was a problem hiding this comment.
Pull request overview
This PR fixes MADE.Threading.Timer due-time scheduling behavior and hardens parts of the networking/diagnostics stack by improving nullability handling, async processing, and adding new test coverage projects.
Changes:
- Fix timer start due-time handling (store
DueTime, useTotalMillisecondsrather than the millisecond component). - Update networking request APIs for nullable headers/data, reuse shared JSON serializer options, and modernize
NetworkRequestManagerqueue processing to async. - Add new
MADE.Foundation.TestsandMADE.Runtime.Testsprojects (plus new test suites), and update solution structure accordingly.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MADE.Threading.Tests/Tests/TimerTests.cs | Adds coverage for timer start/stop/dispose and due-time behavior. |
| tests/MADE.Runtime.Tests/Tests/WeakReferenceEventListenerWithEventArgsTests.cs | Adds tests for weak event listener (with event args). |
| tests/MADE.Runtime.Tests/Tests/WeakReferenceEventListenerTests.cs | Adds tests for weak event listener (no event args). |
| tests/MADE.Runtime.Tests/Tests/WeakReferenceCallbackTests.cs | Adds tests for WeakReferenceCallback. |
| tests/MADE.Runtime.Tests/MADE.Runtime.Tests.csproj | Introduces new runtime test project. |
| tests/MADE.Foundation.Tests/Tests/PlatformNotSupportedExceptionTests.cs | Adds tests for platform exception type. |
| tests/MADE.Foundation.Tests/Tests/PlatformNotSupportedAttributeTests.cs | Adds tests for platform attribute usage metadata. |
| tests/MADE.Foundation.Tests/Tests/PlatformApiHelperTests.cs | Adds tests for API support helper behavior. |
| tests/MADE.Foundation.Tests/MADE.Foundation.Tests.csproj | Introduces new foundation test project. |
| src/MADE.Threading/Timer.cs | Fixes due-time conversion and persists DueTime when starting. |
| src/MADE.Networking/MADE.Networking.csproj | Removes previous NoWarn suppression list. |
| src/MADE.Networking/Http/Responses/HttpResponseMessage{T}.cs | Makes response properties nullable-friendly (e.g., ReasonPhrase, RequestMessage, DeserializedContent). |
| src/MADE.Networking/Http/Requests/Streams/StreamGetNetworkRequest.cs | Allows nullable headers in stream GET request constructor. |
| src/MADE.Networking/Http/Requests/NetworkRequestCallback.cs | Allows nullable success/error callbacks. |
| src/MADE.Networking/Http/Requests/NetworkRequest.cs | Allows nullable headers in base request ctor and normalizes to empty dictionary. |
| src/MADE.Networking/Http/Requests/MultipartFormDataPostNetworkRequest.cs | Reuses default JSON options; keeps multipart POST logic. |
| src/MADE.Networking/Http/Requests/Json/JsonPutNetworkRequest.cs | Nullable JSON body support + shared JSON options + cancellation-aware reads. |
| src/MADE.Networking/Http/Requests/Json/JsonPostNetworkRequest.cs | Nullable JSON body support + shared JSON options + cancellation-aware reads. |
| src/MADE.Networking/Http/Requests/Json/JsonPatchNetworkRequest.cs | Nullable JSON body support + shared JSON options + cancellation-aware reads. |
| src/MADE.Networking/Http/Requests/Json/JsonGetNetworkRequest.cs | Shared JSON options + cancellation-aware reads. |
| src/MADE.Networking/Http/Requests/Json/JsonDeleteNetworkRequest.cs | Shared JSON options + cancellation-aware reads. |
| src/MADE.Networking/Http/NetworkRequestManager.cs | Converts queue processing to async, adjusts callback nullability handling, and hardens timer tick callback. |
| src/MADE.Networking/Http/NetworkRequestFactory.cs | Removes null-forgiving usage when passing optional headers/body. |
| src/MADE.Networking/Http/INetworkRequestManager.cs | Renames queue processing API to ProcessCurrentQueueAsync. |
| src/MADE.Networking/Extensions/UriExtensions.cs | Makes GetQueryValue return nullable string. |
| src/MADE.Diagnostics/AppDiagnostics.cs | Wraps last-resort exception handlers in try/catch to avoid process crashes. |
| MADE.NET.sln | Adds new test projects to solution; updates solution structure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace generic catch clauses with catch (Exception) in AppDiagnostics and NetworkRequestManager. Remove redundant null check on successCallback in NetworkRequestManager catch block. Guard nullable Data property in Post/Put/Patch requests with fallback to empty string. Add null checks after JsonSerializer.Deserialize in all JSON request types. Use Assume.That for non-deterministic GC tests in WeakReferenceCallbackTests. Remove unused detached variable and add explicit assertion in WeakReferenceEventListenerWithEventArgsTests. Move helper types into PlatformApiHelperTests as private nested classes. Restore solution file BOM and remove leading blank line. Update misleading catch comment in timer callback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several improvements and fixes across the networking and diagnostics components, as well as updates to the solution file. The most significant changes include making network request APIs more robust and nullable-friendly, improving exception handling to prevent process crashes, and modernizing asynchronous processing in the network request manager.
Networking API improvements:
JsonGetNetworkRequest,JsonPostNetworkRequest,JsonPatchNetworkRequest,JsonDeleteNetworkRequest) to accept nullable headers and data parameters, improving null-safety and flexibility. [1] [2] [3] [4] [5] [6]DefaultJsonOptionsinstance with case-insensitive property names in all JSON network requests. [1] [2] [3] [4] [5] [6] [7]ProcessCurrentQueueto an async methodProcessCurrentQueueAsyncinNetworkRequestManagerand its interface, and updated the timer callback to handle exceptions safely. [1] [2] [3] [4]Diagnostics robustness:
AppDiagnosticsby wrapping last-resort exception handlers in try/catch blocks to prevent process crashes from unhandled exceptions. [1] [2] [3]Solution and project structure:
MADE.Foundation.TestsandMADE.Runtime.Teststo the solution, including their build and solution folder configuration. [1] [2] [3]Other minor improvements include making the
GetQueryValueextension method return a nullable string and passing cancellation tokens toReadAsStringAsyncfor better cancellation support. [1] [2] [3]