Skip to content

Conversation

@MarkCiliaVincenti
Copy link
Contributor

@MarkCiliaVincenti MarkCiliaVincenti commented Aug 21, 2024

Summary by CodeRabbit

  • New Features

    • Introduced a new Lock class for enhanced thread safety and synchronization across various components.
    • Updated locking mechanisms in multiple classes to utilize the new Lock type, improving concurrency control.
  • Bug Fixes

    • Improved thread safety in classes such as FileWatcher, DatabaseWatcher, and Bootstrapper through refined locking strategies.
  • Documentation

    • Updated documentation to reflect changes in locking mechanisms and their benefits for concurrency management.

@gimlichael
Copy link
Member

Thank you for your contribution - I will review it and might add-in some preprocessor-directives for backward compatibility.

@MarkCiliaVincenti
Copy link
Contributor Author

Hi,

You don't really need any preprpcessor directives. The micro dependency added takes care of that. Furthermore, the dependency is only added for frameworks earlier than .NET 9.0

@gimlichael
Copy link
Member

I like your proposed changes - but since Cuemon extends upon Microsoft .NET, one rule is to not have dependencies to other assemblies. Unless they are explicit made so, e.g., :

  • Cuemon.Extensions.Newtonsoft.Json
  • Cuemon.Extensions.Swashbuckle.AspNetCore
  • Cuemon.Extensions.Xunit
  • Cuemon.Extensions.YamlDotNet

So unless you feel for it, this PR can only be merged without external dependency, hence only be made available for .NET 9.

BTW; cool little library you have written - kudos!

@MarkCiliaVincenti
Copy link
Contributor Author

I understand, though it is hardly a dependency in the sense that it is merely making calls to methods that are part of the framework.

It is MIT though so you can copy in the single class file, although credit to the library would be appreciated. You'll miss out on potential updates though this way.

@gimlichael
Copy link
Member

I will consider it for sure - and credits are self-evident. Thank you for your understanding.

@coderabbitai
Copy link

coderabbitai bot commented Aug 26, 2024

Walkthrough

The changes involve several modifications across various files in the project. Key updates include changing the LangVersion in the Directory.Build.props file from latest to preview, and updating locking fields from object to a new Lock type in multiple classes. These adjustments reflect a focus on enhancing thread safety and synchronization mechanisms throughout the codebase.

Changes

File(s) Change Summary
Directory.Build.props Updated <LangVersion> from latest to preview.
src/Cuemon.Core/Cuemon.Core.csproj Added an ItemGroup to conditionally exclude Lock.cs from compilation for the net9.0 target framework.
src/Cuemon.Core/Runtime/Dependency.cs, src/Cuemon.Core/Runtime/FileWatcher.cs, src/Cuemon.Core/Runtime/Watcher.cs, src/Cuemon.Data/DatabaseWatcher.cs, src/Cuemon.Extensions.Newtonsoft.Json/Formatters/NewtonsoftJsonFormatterOptions.cs, src/Cuemon.Extensions.Text.Json/Formatters/JsonFormatterOptions.cs, src/Cuemon.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs, src/Cuemon.Xml/Serialization/Formatters/XmlFormatterOptions.cs Changed _locker type from object to Lock.
src/Cuemon.Data/BulkCopyDataReader.cs Changed PadLock type from object to Lock.
src/Cuemon.Extensions.AspNetCore.Newtonsoft.Json/Bootstrapper.cs, src/Cuemon.Extensions.AspNetCore.Text.Json/Bootstrapper.cs, src/Cuemon.Extensions.AspNetCore.Xml/Bootstrapper.cs Changed PadLock type from object to Lock.
src/Cuemon.Core/Threading/Lock.cs Introduced a new Lock class with methods for enhanced locking mechanisms.

Sequence Diagram(s)

sequenceDiagram
    participant Dev as Developer
    participant Repo as Code Repository
    participant Build as Build System
    participant App as Application

    Dev->>Repo: Push changes
    Repo->>Build: Trigger build process
    Build->>App: Compile with new settings
    App->>App: Use new Lock type for synchronization
    App->>App: Utilize preview language features
Loading

🐇 In the code, a shift we see,
From object locks to Lock, oh me!
With threads now safe, we hop and play,
In preview lands, we find our way.
So let’s rejoice, for changes bright,
A bouncin’ leap, in code's delight! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@MarkCiliaVincenti
Copy link
Contributor Author

I will consider it for sure - and credits are self-evident. Thank you for your understanding.

Check now.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)
src/Cuemon.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs (1)

19-19: Potential Issue with Lock Usage in lock Statement

The Lock class is implemented with methods that utilize Monitor.Enter, but there is a warning indicating that using this class in a lock statement might lead to unintended behavior. This could affect thread safety and should be reviewed.

  • File: src/Cuemon.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs
  • Line: Usage of _locker in a lock statement.

Please review the implementation and usage of the Lock class to ensure it aligns with its intended use.

Analysis chain

LGTM! But verify the correct usage of the Lock type.

The change to the Lock type likely enhances thread safety or provides more specialized locking behavior. Ensure that the Lock type is used correctly in the rest of the class.

Run the following script to verify the correct usage of the Lock type in the rest of the class:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correct usage of the `Lock` type in the `YamlFormatterOptions` class.

# Test: Search for the usage of `_locker`. Expect: Correct usage of the `Lock` type.
rg --type cs '_locker' src/Cuemon.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs

Length of output: 158


Script:

#!/bin/bash
# Search for the definition of the `Lock` type to verify its compatibility with the `lock` statement.
ast-grep --lang csharp --pattern 'class Lock { $$$ }'

Length of output: 55


Script:

#!/bin/bash
# Search for any using directives that might import the `Lock` type.
rg --type cs 'using' src/Cuemon.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs

# Search for any class definitions that might match `Lock` under a different namespace or name.
rg --type cs 'class Lock' 

Length of output: 444


Script:

#!/bin/bash
# Retrieve the implementation details of the `Lock` class to verify its compatibility with the `lock` statement.
rg --type cs -A 20 'class Lock' src/Cuemon.Core/Threading/Lock.cs

Length of output: 910

src/Cuemon.Core/Threading/Lock.cs (1)

1-4: Add license information.

Consider adding a license header to the file to ensure proper attribution and compliance with the MIT license.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e251523 and a1e6453.

Files selected for processing (17)
  • Directory.Build.props (1 hunks)
  • src/Cuemon.Core/Cuemon.Core.csproj (1 hunks)
  • src/Cuemon.Core/Runtime/Dependency.cs (2 hunks)
  • src/Cuemon.Core/Runtime/FileWatcher.cs (2 hunks)
  • src/Cuemon.Core/Runtime/Watcher.cs (1 hunks)
  • src/Cuemon.Core/Threading/Lock.cs (1 hunks)
  • src/Cuemon.Data/BulkCopyDataReader.cs (2 hunks)
  • src/Cuemon.Data/DatabaseWatcher.cs (2 hunks)
  • src/Cuemon.Extensions.AspNetCore.Newtonsoft.Json/Bootstrapper.cs (1 hunks)
  • src/Cuemon.Extensions.AspNetCore.Text.Json/Bootstrapper.cs (1 hunks)
  • src/Cuemon.Extensions.AspNetCore.Xml/Bootstrapper.cs (1 hunks)
  • src/Cuemon.Extensions.Net/Http/SlimHttpClientFactory.cs (1 hunks)
  • src/Cuemon.Extensions.Newtonsoft.Json/Formatters/NewtonsoftJsonFormatterOptions.cs (2 hunks)
  • src/Cuemon.Extensions.Runtime.Caching/CacheEnumerableExtensions.cs (2 hunks)
  • src/Cuemon.Extensions.Text.Json/Formatters/JsonFormatterOptions.cs (2 hunks)
  • src/Cuemon.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs (2 hunks)
  • src/Cuemon.Xml/Serialization/Formatters/XmlFormatterOptions.cs (2 hunks)
Additional comments not posted (37)
src/Cuemon.Extensions.AspNetCore.Text.Json/Bootstrapper.cs (2)

3-3: LGTM!

The import System.Threading is necessary for using the Lock class.

The code changes are approved.


9-9: LGTM!

Changing the PadLock variable type from object to Lock enhances the locking mechanism, improving thread safety and synchronization.

The code changes are approved.

src/Cuemon.Extensions.AspNetCore.Newtonsoft.Json/Bootstrapper.cs (2)

3-3: LGTM!

The import System.Threading is necessary for using the Lock class.

The code changes are approved.


9-9: LGTM!

Changing the PadLock variable type from object to Lock enhances the locking mechanism, improving thread safety and synchronization.

The code changes are approved.

src/Cuemon.Extensions.AspNetCore.Xml/Bootstrapper.cs (2)

3-3: LGTM!

The import System.Threading is necessary for using the Lock class.

The code changes are approved.


9-9: LGTM!

Changing the PadLock variable type from object to Lock enhances the locking mechanism, improving thread safety and synchronization.

The code changes are approved.

src/Cuemon.Core/Cuemon.Core.csproj (1)

16-18: LGTM!

The conditional compilation to remove Lock.cs when targeting net9.0 is a good practice for maintaining compatibility and optimizing the build process.

The code changes are approved.

src/Cuemon.Core/Runtime/FileWatcher.cs (1)

3-3: LGTM!

Changing the _locker field type to Lock enhances thread safety and synchronization within the FileWatcher class. This aligns with best practices in concurrent programming.

The code changes are approved.

Also applies to: 15-15

src/Cuemon.Data/DatabaseWatcher.cs (1)

5-5: LGTM!

Changing the _locker field type to Lock enhances thread safety and synchronization within the DatabaseWatcher class. This aligns with best practices in concurrent programming.

The code changes are approved.

Also applies to: 18-18

Directory.Build.props (1)

8-8: Verify the necessity of using the preview language version.

Changing the LangVersion to preview can introduce experimental or unreleased functionalities, which might lead to potential instability or breaking changes. Ensure that this change is necessary and that the project is prepared for any issues that might arise from using a preview version.

src/Cuemon.Core/Runtime/Dependency.cs (2)

3-3: LGTM!

The addition of the using System.Threading; directive is necessary to use the Lock type.


16-16: LGTM! But verify the correct usage of the Lock type.

The change to the Lock type likely enhances thread safety or provides more specialized locking behavior. Ensure that the Lock type is used correctly in the rest of the class.

Run the following script to verify the correct usage of the Lock type in the rest of the class:

Verification successful

Correct usage of the Lock type verified.

The Lock class is appropriately designed to be used with the lock statement, and its usage in the Dependency class is correct. The change enhances thread safety by using a more specialized locking mechanism.

  • src/Cuemon.Core/Runtime/Dependency.cs: Usage of _locker in a lock statement is correct.
  • src/Cuemon.Core/Threading/Lock.cs: The Lock class is implemented to support monitor-based locking.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correct usage of the `Lock` type in the `Dependency` class.

# Test: Search for the usage of `_locker`. Expect: Correct usage of the `Lock` type.
rg --type cs '_locker' src/Cuemon.Core/Runtime/Dependency.cs

Length of output: 138


Script:

#!/bin/bash
# Verify if there are any other usages of `_locker` in the `Dependency` class.
rg --type cs '_locker' -A 5 src/Cuemon.Core/Runtime/Dependency.cs

# Check the implementation of the `Lock` class to ensure it is suitable for use in a `lock` statement.
ast-grep --lang csharp --pattern 'class Lock { $$$ }'

Length of output: 612


Script:

#!/bin/bash
# Search for the definition of the `Lock` class in the codebase.
rg --type cs 'class Lock' -A 10

Length of output: 909

src/Cuemon.Extensions.YamlDotNet/Formatters/YamlFormatterOptions.cs (1)

5-5: LGTM!

The addition of the using System.Threading; directive is necessary to use the Lock type.

src/Cuemon.Extensions.Net/Http/SlimHttpClientFactory.cs (1)

32-32: LGTM!

Changing the _locker field type from object to Lock improves thread safety and synchronization.

The code changes are approved.

src/Cuemon.Extensions.Text.Json/Formatters/JsonFormatterOptions.cs (1)

20-20: LGTM!

Changing the _locker field type from object to Lock improves thread safety and synchronization.

The code changes are approved.

src/Cuemon.Xml/Serialization/Formatters/XmlFormatterOptions.cs (1)

17-17: LGTM!

Changing the _locker field type from object to Lock improves thread safety and synchronization.

The code changes are approved.

src/Cuemon.Core/Threading/Lock.cs (13)

5-7: Security transparency attribute.

The SecurityTransparent attribute is used to indicate that the assembly cannot cause an elevation of privilege. Ensure this is the intended behavior.


8-10: Type forwarding for .NET 9.0+.

The TypeForwardedTo attribute ensures that the Lock type is forwarded to the .NET 9.0+ implementation. This is a good practice to maintain compatibility.


11-12: Namespace declaration.

The System.Threading namespace is used to align with the .NET 9.0+ implementation. This is a good practice for consistency.


13-22: Class documentation.

The class documentation is thorough and provides a clear explanation of the Lock class and its behavior. This is a good practice for maintainability.


24-24: Suppress warning CS9216.

The warning suppression ensures that the Lock class is not mistakenly used with a lock statement. This is a good practice to prevent unintended behavior.


25-32: Enter method.

The Enter method uses Monitor.Enter to acquire the lock. The method is correctly implemented and uses AggressiveInlining for performance optimization.


34-44: TryEnter method.

The TryEnter method uses Monitor.TryEnter to attempt to acquire the lock. The method is correctly implemented and uses AggressiveInlining for performance optimization.


46-57: TryEnter with timeout method.

The TryEnter method with a TimeSpan parameter uses Monitor.TryEnter to attempt to acquire the lock with a timeout. The method is correctly implemented and uses AggressiveInlining for performance optimization.


59-70: TryEnter with milliseconds timeout method.

The TryEnter method with an int parameter uses Monitor.TryEnter to attempt to acquire the lock with a timeout in milliseconds. The method is correctly implemented and uses AggressiveInlining for performance optimization.


72-80: Exit method.

The Exit method uses Monitor.Exit to release the lock. The method is correctly implemented and uses AggressiveInlining for performance optimization.


82-93: IsHeldByCurrentThread property.

The IsHeldByCurrentThread property uses Monitor.IsEntered to determine if the current thread holds the lock. The property is correctly implemented and provides a fallback for unsupported .NET versions.


96-117: EnterScope method.

The EnterScope method acquires the lock and returns a disposable Scope struct. The method is correctly implemented and uses AggressiveInlining for performance optimization.


119-138: Scope struct.

The Scope struct is a disposable structure that exits the lock when disposed. The struct is correctly implemented and uses AggressiveInlining for performance optimization.

src/Cuemon.Core/Runtime/Watcher.cs (3)

13-13: Field type change to Lock.

The _locker field type has been changed from object to Lock. This change enhances thread safety and synchronization mechanisms within the class.


13-13: Initialize _locker field.

The _locker field is initialized using the Lock constructor. This ensures that the field is properly set up for synchronization.


13-13: Use of lock statement with Lock.

The lock statement is used with the _locker field of type Lock. Ensure that the Lock class is compatible with the lock statement and does not cause unintended behavior.

src/Cuemon.Extensions.Newtonsoft.Json/Formatters/NewtonsoftJsonFormatterOptions.cs (3)

20-20: Field type change to Lock.

The _locker field type has been changed from object to Lock. This change enhances thread safety and synchronization mechanisms within the class.


20-20: Initialize _locker field.

The _locker field is initialized using the Lock constructor. This ensures that the field is properly set up for synchronization.


20-20: Use of lock statement with Lock.

The lock statement is used with the _locker field of type Lock. Ensure that the Lock class is compatible with the lock statement and does not cause unintended behavior.

src/Cuemon.Data/BulkCopyDataReader.cs (1)

9-9: LGTM!

The change from object to Lock for the PadLock field enhances thread safety by using a more robust locking mechanism.

The code changes are approved.

Also applies to: 18-18

src/Cuemon.Extensions.Runtime.Caching/CacheEnumerableExtensions.cs (1)

6-6: LGTM!

The change from object to Lock for the PadLock field enhances thread safety by using a more robust locking mechanism.

The code changes are approved.

Also applies to: 710-710

@MarkCiliaVincenti
Copy link
Contributor Author

@gimlichael note that on .NET 9.0+, the added class will even be excluded from compilation.

@gimlichael gimlichael self-assigned this Aug 26, 2024
@gimlichael
Copy link
Member

gimlichael commented Aug 26, 2024

@MarkCiliaVincenti I just try to run the unit-test locally - and fairly quickly they start to fail. Could I convince you to try and run the tests?

I received this from FileDependencyTest for net48:

Attempt by security transparent method 'Cuemon.Threading.TimerFactory.CreateNonCapturingTimer(System.Threading.TimerCallback, System.Object, System.TimeSpan, System.TimeSpan)' to access security critical method 'System.Threading.ExecutionContext.SuppressFlow()' failed.

PS
I first have time tomorrow again - so no rush.

@MarkCiliaVincenti
Copy link
Contributor Author

Hmm I will take a look.

I thought this was there to prevent exactly this error:
[assembly: SecurityTransparent()]

Maybe it's the one actually causing it?

@gimlichael
Copy link
Member

Could be - since the attribute only affects .NET Framework .. and looks like an escalation of privilege.
Its been a while since I have looked at these specialized attributes in .NET Framework context .. I can try to remove it after the test-run complete .. so far 22 failed tests. Hooray for tests!

@MarkCiliaVincenti
Copy link
Contributor Author

MarkCiliaVincenti commented Aug 26, 2024 via email

@gimlichael
Copy link
Member

That is a good point - and sometime in .NET Framework context extra privilege might be granted. If I have more energy tomorrow, I will try to poke around as it has awaken my curiosity.

Btw; when I removed the attribute, the tests passes ✅

@MarkCiliaVincenti
Copy link
Contributor Author

Very interesting. I'm trying to play around but perhaps not a good idea to use an 11 year old laptop.

Thing is, forget this PR... what happens if an assembly tries to use Cuemon with extra privileges? Will it complain?

@MarkCiliaVincenti
Copy link
Contributor Author

Another interesting experiment. I reverted to using the NuGet package, and everything worked.

I don't have enough knowledge about this subject to advise whether it is more advisable to segregate it into its own assembly, but I'm wondering if you'd reconsider adding in the micro dependency.

If not, we can try to remove that line as you wrote, but I'm a bit more wary about the repercussions of such a change, but it's just my gut feeling.

@gimlichael
Copy link
Member

That would probably be a use-case to play around with. The original source is from Microsoft: https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Extensions/NonCapturingTimer/NonCapturingTimer.cs which is used by a number of assembly packages that also targets .NET Framework. Interestingly, they have not decorated anywhere with [SecuritySafeCritical] which Copilot suggest might be a solution. Then again; this was the "old" way with object as a PadLock.


The error message you're encountering is related to .NET's security model, specifically around the concept of security transparency. Here's a breakdown of the key components:

Security Transparency

• Security Transparent Methods: These methods cannot perform security-critical operations. They are considered safe and cannot elevate privileges or access sensitive resources.
• Security Critical Methods: These methods can perform operations that require elevated privileges and can access sensitive resources.

The Error

The error message indicates that a security transparent method (Cuemon.Threading.TimerFactory.CreateNonCapturingTimer) is attempting to call a security critical method (System.Threading.ExecutionContext.SuppressFlow). This is not allowed under .NET's security model.

Why This Happens

• Security Transparent Code: The method CreateNonCapturingTimer is marked as security transparent, meaning it should not perform any security-sensitive operations.
• Security Critical Code: The method SuppressFlow is marked as security critical, meaning it performs operations that require higher privileges.

How to Resolve

  1. Review Method Attributes: Ensure that the method CreateNonCapturingTimer is correctly marked with the appropriate security attributes. If it needs to call security critical methods, it should be marked as security critical or security safe-critical.
  2. Refactor Code: If possible, refactor the code to avoid calling security critical methods from security transparent methods.
  3. Update Security Policies: Ensure that your project’s security policies and attributes are correctly configured to allow the necessary operations.

@MarkCiliaVincenti
Copy link
Contributor Author

How do you want to do this? Do you want to:

a) remove that attribute from Lock.cs
or
b) re-add the dependency to Backport.System.Threading.Lock?

Either way allow those tests to work successfully. Option B sounds safer to me, but you had some reservations about adding in a new dependency.

Let me know and I'll amend the PR accordingly.

@gimlichael
Copy link
Member

I need to sleep on it, but for sure no dependency from the core assembly. I might tweak for legacy .NET Framework.

@MarkCiliaVincenti
Copy link
Contributor Author

MarkCiliaVincenti commented Aug 26, 2024

Not sure what you mean, something like this?

<ItemGroup Condition="'$(TargetFramework)' == 'net45'">
  <PackageReference Include="Backport.System.Threading.Lock" Version="1.1.6" />
  <Compile Remove="Threading\Lock.cs" />
</ItemGroup>

PS: that condition is probably wrong.

@gimlichael
Copy link
Member

No, more like removing all logic from Lock.cs .. so basically it's just an empty object if net48. This should keep backward compatibility to legacy .NET Framework.

@MarkCiliaVincenti
Copy link
Contributor Author

But then you can't call the methods available on System.Threading.Lock. In fact, you'd only be able to use lock (syncRoot), nothing else. You can't even use Monitor.Enter on a Lock object, that's why it exposes its own Enter method for example.

@MarkCiliaVincenti
Copy link
Contributor Author

Another option to consider is to create a new project called Backport.System.Threading.Lock and literally copy everything in there rather than rely on a Lock.cs in the same assembly. It would also be easier to update in case of changes. As long as you credit it, I'm fine with that as well.

@gimlichael
Copy link
Member

Correct, but for legacy .NET I think backward compatibility is more important. Cuemon was originally NET Framework 2 and 4 CLR additions, that then matured almost purely to .NET (core). However, many assemblies still work with net48 and netstandard2.0.

@MarkCiliaVincenti
Copy link
Contributor Author

Correct, but for legacy .NET I think backward compatibility is more important. Cuemon was originally NET Framework 2 and 4 CLR additions, that then matured almost purely to .NET (core). However, many assemblies still work with net48 and netstandard2.0.

I understand, but are there any scenarios where having Backport.System.Threading.Lock as a project in your solution give any issues under net48 or earlier? I fiddled around with different scenarios and I couldn't find any issue.

Are your concerns borne out of "just in case" or is there an actual issue? I'm asking because if there is I'd obviously like to fix it.

@gimlichael
Copy link
Member

Its more a principle than a concern. As mentioned earlier, exceptions are though that outside of the confines of Microsoft, where I add a Cuemon.Extensions.3rdParty .. at work now .. will get back .. .NET is more important than .NET Legacy .. haha .. that is why I think the safer approach is simply having an empty class - otherwise the changes will be to vast, I think.

@gimlichael
Copy link
Member

@MarkCiliaVincenti I have researched a little further. I think you just need to :remove SecurityTransparent attribute as this should not apply to the Cuemon.Core assembly: https://learn.microsoft.com/en-us/dotnet/api/system.security.securitytransparentattribute

Then it should be good to go.

@MarkCiliaVincenti
Copy link
Contributor Author

Done.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a1e6453 and 1cafc5f.

Files selected for processing (1)
  • src/Cuemon.Core/Threading/Lock.cs (1 hunks)
Files skipped from review due to trivial changes (1)
  • src/Cuemon.Core/Threading/Lock.cs

@gimlichael
Copy link
Member

Apologies, but could you update your fork with the last two commits? The latest one fixes a security related feature, that should allow my CI to run using pull_request_target instead of pull_request trigger. Sorry for the inconvenience. More info.: https://www.codu.co/articles/when-to-use-pull_request_target-instead-of-pull_request-in-github-actions-k3pao1qz

@MarkCiliaVincenti
Copy link
Contributor Author

Synced fork

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1cafc5f and 0edf06b.

Files selected for processing (6)
  • src/Cuemon.Extensions.AspNetCore.Newtonsoft.Json/Bootstrapper.cs (1 hunks)
  • src/Cuemon.Extensions.AspNetCore.Text.Json/Bootstrapper.cs (1 hunks)
  • src/Cuemon.Extensions.AspNetCore.Xml/Bootstrapper.cs (1 hunks)
  • src/Cuemon.Extensions.Newtonsoft.Json/Formatters/NewtonsoftJsonFormatterOptions.cs (2 hunks)
  • src/Cuemon.Extensions.Text.Json/Formatters/JsonFormatterOptions.cs (2 hunks)
  • src/Cuemon.Xml/Serialization/Formatters/XmlFormatterOptions.cs (2 hunks)
Files skipped from review due to trivial changes (1)
  • src/Cuemon.Extensions.Text.Json/Formatters/JsonFormatterOptions.cs
Files skipped from review as they are similar to previous changes (5)
  • src/Cuemon.Extensions.AspNetCore.Newtonsoft.Json/Bootstrapper.cs
  • src/Cuemon.Extensions.AspNetCore.Text.Json/Bootstrapper.cs
  • src/Cuemon.Extensions.AspNetCore.Xml/Bootstrapper.cs
  • src/Cuemon.Extensions.Newtonsoft.Json/Formatters/NewtonsoftJsonFormatterOptions.cs
  • src/Cuemon.Xml/Serialization/Formatters/XmlFormatterOptions.cs

@MarkCiliaVincenti
Copy link
Contributor Author

These failures look unrelated, right?

@codecov
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.21%. Comparing base (a539fa1) to head (0edf06b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #73   +/-   ##
=======================================
  Coverage   80.21%   80.21%           
=======================================
  Files         707      707           
  Lines       21586    21586           
  Branches     2177     2177           
=======================================
  Hits        17315    17315           
  Misses       4218     4218           
  Partials       53       53           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gimlichael gimlichael merged commit 450b22e into codebeltnet:main Aug 29, 2024
@gimlichael
Copy link
Member

Merged - thank you for your contribution!

@MarkCiliaVincenti
Copy link
Contributor Author

Hi @gimlichael, I have become aware that the lock implementation was not hardened against thread aborts. Given that you are targeting netstandard2.0 it means you are affected. On pre-.NET 5.0, someone could call a Thread.Abort() at a very short time window between when the lock is obtained and it entering a try lock, leaving the lock object in a locked state.

I suggest you go for these files instead:
https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/LockFactory.cs
https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/PreNet5Lock.cs

and then you can have this in your csproj files or Directory.Build.props:

<ItemGroup>
  <Using Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="System.Threading.Lock" />
  <Using Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="Backport.System.Threading.Lock" />
  <Using Alias="LockFactory" Include="Backport.System.Threading.LockFactory" />
</ItemGroup>

@MarkCiliaVincenti
Copy link
Contributor Author

You would then need to change from new Lock() to LockFactory.Create()

@gimlichael
Copy link
Member

Hi @gimlichael, I have become aware that the lock implementation was not hardened against thread aborts. Given that you are targeting netstandard2.0 it means you are affected. On pre-.NET 5.0, someone could call a Thread.Abort() at a very short time window between when the lock is obtained and it entering a try lock, leaving the lock object in a locked state.

I suggest you go for these files instead: https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/LockFactory.cs https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/PreNet5Lock.cs

and then you can have this in your csproj files or Directory.Build.props:

<ItemGroup>
  <Using Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="System.Threading.Lock" />
  <Using Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="Backport.System.Threading.Lock" />
  <Using Alias="LockFactory" Include="Backport.System.Threading.LockFactory" />
</ItemGroup>

@MarkCiliaVincenti - to keep things simple - would this fix work: https://github.com/gimlichael/Cuemon/blob/v9.0.0/lock-refactoring/src/Cuemon.Core/Threading/Lock.cs

If not, then I will remove the #else and update those assemblies that targets netstandard2.0 to use new object (as before) and simply only provide Lock surrogate for >= NET5.0 && <= .NET8.0.

Thoughts?

@MarkCiliaVincenti
Copy link
Contributor Author

I'm not convinced. I will take a look tomorrow. But would you consider putting it into its own project? I think it would be much simpler if you do especially if the library undergoes more changes in future. Certain things need to be done with caution like the use of that type forwarding for example.

@MarkCiliaVincenti
Copy link
Contributor Author

Also not sure what you meant re targets. If you only target net5 and above it would be simpler yes. You can also do without the type forwarding as long as you do target net9 directly as well.

@gimlichael
Copy link
Member

Also not sure what you meant re targets. If you only target net5 and above it would be simpler yes. You can also do without the type forwarding as long as you do target net9 directly as well.

What I mean is to simply use new object() instead of new Lock() in those libraries that targets netstandard2.0.
For those non-compliant with netstandard2.0, I would continue to use your Lock implementation.

@gimlichael
Copy link
Member

I'm not convinced. I will take a look tomorrow. But would you consider putting it into its own project? I think it would be much simpler if you do especially if the library undergoes more changes in future. Certain things need to be done with caution like the use of that type forwarding for example.

That might be a good idea; I do have https://github.com/gimlichael/Cuemon/tree/main/src/Cuemon.Threading that might be a good fit for this addition. As you allude to, it might be a cleaner cut .. still not able to reference your library directly, but the maintenance would be moved from "core" to "complemental".

That written .. if the proposal seems to work, maybe its good enough?
Only reason I was hesitant was due to your factory implementation and writing in the notes that this class should not be instantiated on its own .. but I figure that is due to the factory, which I skipped in favor of a more "clean" though redundant implementation.

@MarkCiliaVincenti
Copy link
Contributor Author

Because of the TypeForwardedTo (AssemblyInfo.cs) it should be in its own assembly.

Basically in something that's .NET 5.0+ you can do this (replace PackageReference with ProjectReference if you don't want to depend on the library):

<ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))">
  <PackageReference Include="Backport.System.Threading.Lock" Version="2.0.0" />  
</ItemGroup>

...

private readonly Lock _syncRoot = new();

and if using something that targets anything prior to .NET 5.0:

<ItemGroup>
  <PackageReference Include="Backport.System.Threading.Lock" Version="2.0.0" />
  <Using Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="System.Threading.Lock" />
  <Using Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="Backport.System.Threading.Lock" />
  <Using Alias="LockFactory" Include="Backport.System.Threading.LockFactory" />
</ItemGroup>

...

private readonly Lock _syncRoot = LockFactory.Create();

Up to you whichever way you want to take this.

@gimlichael
Copy link
Member

gimlichael commented Sep 9, 2024

Because of the TypeForwardedTo (AssemblyInfo.cs) it should be in its own assembly.

Basically in something that's .NET 5.0+ you can do this (replace PackageReference with ProjectReference if you don't want to depend on the library):

<ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))">
  <PackageReference Include="Backport.System.Threading.Lock" Version="2.0.0" />  
</ItemGroup>

...

private readonly Lock _syncRoot = new();

and if using something that targets anything prior to .NET 5.0:

<ItemGroup>
  <PackageReference Include="Backport.System.Threading.Lock" Version="2.0.0" />
  <Using Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="System.Threading.Lock" />
  <Using Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="Backport.System.Threading.Lock" />
  <Using Alias="LockFactory" Include="Backport.System.Threading.LockFactory" />
</ItemGroup>

...

private readonly Lock _syncRoot = LockFactory.Create();

Up to you whichever way you want to take this.

I will reference your assembly in a hidden gem of mine; Cuemon.Extensions.Threading. If possible, I would encourage you to see if its possible to skip the LockFactory.Create() for consistency. That said; this option does not violate principles of Cuemon - and it embraces your library extension for .NET.

I will make a PR later that you are welcome to approve.

@gimlichael gimlichael mentioned this pull request Sep 9, 2024
@MarkCiliaVincenti
Copy link
Contributor Author

The reason for LockFactory.Create() is that it is dangerous to provide an instance called System.Threading.Lock to pre .NET 5.0, because when you lock on it, on a .NET 8 or .NET 9 compiler it will end up compiling the code in a way that is not hardened against thread aborts; basically if a thread is aborted exactly after it obtains a lock and before it starts doing whatever needs to happen inside the lock, it would leave the lock object locked.

LockFactory.Create() is there so that if you're on .NET 9.0+ you get System.Threading.Lock and if you're anything before that you get an instance of a Backport.System.Threading.Lock, which means that the compiler will compile pre .NET 5.0 code in a way that is hardened against thread aborts.

gimlichael added a commit that referenced this pull request Sep 10, 2024
* 🐛 potential fix of highlighted issue #73 (comment)

* ♻️ refactored Lock to Cuemon.Extensions.Threading and added reference to Backport.System.Threading.Lock
This was referenced Oct 5, 2024
@coderabbitai coderabbitai bot mentioned this pull request Nov 13, 2024
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.

2 participants