Added KeyedSignal that represents a wait-by-key async signaling operation#8
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| LibraryEvents.OnSuppressedException(this, ex); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
The best practice in scenarios requiring exception suppression (e.g., inside Dispose patterns) is to only catch exceptions derived from Exception that are not considered fatal—that is, not system-critical exceptions such as OutOfMemoryException, StackOverflowException, ThreadAbortException, or the base SystemException. The catch-all block should be narrowed by adding a when filter that ignores fatal exceptions, or by explicitly catching only non-fatal exceptions.
In modern .NET, the recommended method is to exclude these fatal exceptions using a helper function, e.g., IsFatal(Exception), to determine if an exception is fatal before handling it/suppressing it. We can introduce this helper, and catch only if the exception is not fatal—otherwise, allow it to propagate.
To implement this, edit the catch block in the Dispose method in src/Gemstone.Threading/KeyedSignal.cs as follows:
- Add a private static helper method, e.g.,
IsFatal(Exception ex), inside the same class. - Change the
catch (Exception ex)line to include awhenclause:catch (Exception ex) when (!IsFatal(ex)).
No additional dependencies are needed.
| @@ -131,7 +131,7 @@ | ||
| { | ||
| m_pruneTask.Wait(); | ||
| } | ||
| catch (Exception ex) | ||
| catch (Exception ex) when (!IsFatal(ex)) | ||
| { | ||
| LibraryEvents.OnSuppressedException(this, ex); | ||
| } | ||
| @@ -403,4 +403,12 @@ | ||
| } | ||
|
|
||
| #endregion | ||
| /// <summary> | ||
| /// Determines if the exception is considered fatal and should not be caught/suppressed. | ||
| /// </summary> | ||
| private static bool IsFatal(Exception ex) => | ||
| ex is OutOfMemoryException | ||
| or StackOverflowException | ||
| or ThreadAbortException | ||
| or AccessViolationException; | ||
| } |
| catch (Exception ex) | ||
| { | ||
| LibraryEvents.OnSuppressedException(this, ex); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the overly broad generic catch clause, narrow the set of caught exceptions in DisposeAsync. The most likely expected exception from await m_pruneTask.ConfigureAwait(false); is OperationCanceledException, given the cancellation logic. Other exceptions are either unexpected or indicative of deeper errors that might better be allowed to propagate or rethrow (or at least have more targeted handling).
Best approach:
- Change
catch (Exception ex)tocatch (OperationCanceledException ex), which aligns with expected cancellation and suppression intent in Dispose. - Optionally, if other exception types are reasonably safe to suppress (e.g.,
ObjectDisposedException), catch and log those specifically. - If you must catch other exceptions (because disposal should never fail), add a second
catchfor genericException ex, log it, but document that this is intentional, or rethrow critical exceptions (such as those inheriting fromSystemError).
Specifically:
- In file
src/Gemstone.Threading/KeyedSignal.cs:- Line 162: Replace
catch (Exception ex)withcatch (OperationCanceledException ex) - Optionally add a broader catch below for unexpected exceptions documenting why they're being suppressed (optional per project policy)
- Line 162: Replace
- No new imports needed.
| @@ -159,10 +159,17 @@ | ||
| { | ||
| await m_pruneTask.ConfigureAwait(false); | ||
| } | ||
| catch (Exception ex) | ||
| catch (OperationCanceledException ex) | ||
| { | ||
| // Suppress expected cancellation exceptions during disposal. | ||
| LibraryEvents.OnSuppressedException(this, ex); | ||
| } | ||
| // Optionally, handle and log other unexpected exceptions. | ||
| // Uncomment below if you wish to report/log all exceptions but not suppress critical errors: | ||
| //catch (Exception ex) when (!(ex is OutOfMemoryException || ex is StackOverflowException || ex is ThreadAbortException)) | ||
| //{ | ||
| // LibraryEvents.OnSuppressedException(this, ex); | ||
| //} | ||
|
|
||
| m_pruneTimer?.Dispose(); | ||
| m_pruneCompletionSource.Dispose(); |
| catch (Exception ex) | ||
| { | ||
| return (default!, ex); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the problem, replace the overly broad catch (Exception ex) block in the Wait method with more specific catch clauses for the exceptions that are part of normal operation and intended error modes. In this context, these likely include OperationCanceledException for cancellations and ObjectDisposedException for disposed object access. Any unhandled exceptions should be allowed to propagate to the caller. Make this change only in the Wait method, lines 212–215 in file src/Gemstone.Threading/KeyedSignal.cs, and do not change other code or imports unless essential.
| @@ -209,10 +209,14 @@ | ||
| TResult result = WaitAsync(key, cancellationToken).ConfigureAwait(false).GetAwaiter().GetResult(); | ||
| return (result, null); | ||
| } | ||
| catch (Exception ex) | ||
| catch (OperationCanceledException ex) | ||
| { | ||
| return (default!, ex); | ||
| } | ||
| catch (ObjectDisposedException ex) | ||
| { | ||
| return (default!, ex); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> |
2 participants
Merge development branch into master