fix memory leak issue#237
Conversation
Reviewer's GuideRefactors the C# script evaluator to cache compiled types using a collectible AssemblyLoadContext, adds cache management APIs, wires the evaluator into DI as a singleton, and updates ScriptEngine to use the injected evaluator to prevent memory leaks from dynamic script compilation. Sequence diagram for script compilation with caching and collectible load contextsequenceDiagram
actor Caller
participant ScriptEngine
participant CSharpEvaluator
participant TypeCache as Type_cache
participant ScriptAssemblyLoadContext as LoadContext
participant Assembly
Caller->>ScriptEngine: ExecuteScriptAsync
ScriptEngine->>CSharpEvaluator: CompileToInstanceAsync_T_(code, extraReferences, usingDirectives, cancellationToken)
CSharpEvaluator->>CSharpEvaluator: GenerateCacheKey(code, targetType, extraReferences, usingDirectives)
CSharpEvaluator->>TypeCache: TryGetValue(cacheKey)
alt cache_hit
TypeCache-->>CSharpEvaluator: (Context, CompiledType)
CSharpEvaluator->>CSharpEvaluator: Activator.CreateInstance(CompiledType)
CSharpEvaluator-->>ScriptEngine: instance
ScriptEngine-->>Caller: result
else cache_miss
CSharpEvaluator->>CSharpEvaluator: Parse syntax, add usings
CSharpEvaluator->>CSharpEvaluator: DefaultMetadataReferences.Value
CSharpEvaluator->>CSharpEvaluator: Create CSharpCompilation
CSharpEvaluator->>CSharpEvaluator: compilation.Emit(memoryStream)
alt emit_failed
CSharpEvaluator-->>ScriptEngine: throw InvalidOperationException
ScriptEngine-->>Caller: error
else emit_success
CSharpEvaluator->>LoadContext: new ScriptAssemblyLoadContext(assemblyName)
CSharpEvaluator->>LoadContext: LoadFromStream(memoryStream)
LoadContext-->>Assembly: loaded_assembly
CSharpEvaluator->>Assembly: GetTypes()
CSharpEvaluator->>CSharpEvaluator: Find type implementing T
alt type_not_found
CSharpEvaluator->>LoadContext: Unload()
CSharpEvaluator-->>ScriptEngine: throw InvalidOperationException
ScriptEngine-->>Caller: error
else type_found
CSharpEvaluator->>TypeCache: TryAdd(cacheKey, (LoadContext, matchedType))
CSharpEvaluator->>CSharpEvaluator: Activator.CreateInstance(matchedType)
CSharpEvaluator-->>ScriptEngine: instance
ScriptEngine-->>Caller: result
end
end
end
Class diagram for updated scripting evaluator and engineclassDiagram
class IEvaluator
class IScriptEngine
class CSharpEvaluator {
- ConcurrentDictionary~string,(AssemblyLoadContext,Type)~ _typeCache
- static Lazy~IReadOnlyList~MetadataReference~~ DefaultMetadataReferences
+ int CachedTypeCount
+ Task~T~ CompileToInstanceAsync_T_(string code, IEnumerable~MetadataReference~ extraReferences, IEnumerable~string~ usingDirectives, CancellationToken cancellationToken)
- Task~T~ CompileAndCacheAsync_T_(string code, string cacheKey, IEnumerable~MetadataReference~ extraReferences, IEnumerable~string~ usingDirectives, CancellationToken cancellationToken)
- static string GenerateCacheKey(string code, Type targetType, IEnumerable~MetadataReference~ extraReferences, IEnumerable~string~ usingDirectives)
- static IReadOnlyList~MetadataReference~ CreateDefaultReferences()
+ void ClearCache()
+ bool InvalidateScript_T_(string code, IEnumerable~MetadataReference~ extraReferences, IEnumerable~string~ usingDirectives)
- static ScriptOptions CreateDefaultOptions(Func~ScriptOptions,ScriptOptions~ configureScriptOptions)
}
class ScriptAssemblyLoadContext {
+ ScriptAssemblyLoadContext(string name)
# Assembly Load(AssemblyName assemblyName)
}
class ScriptEngine {
- IEvaluator _evaluator
+ ScriptEngine(IEvaluator evaluator, IWorkflowMetrics workflowMetrics)
%% other IScriptEngine members not shown
}
IEvaluator <|.. CSharpEvaluator
AssemblyLoadContext <|-- ScriptAssemblyLoadContext
IScriptEngine <|.. ScriptEngine
ScriptEngine --> IEvaluator : uses
CSharpEvaluator --> ScriptAssemblyLoadContext : creates
CSharpEvaluator ..> MetadataReference : uses
CSharpEvaluator ..> ScriptOptions : uses
CSharpEvaluator ..> IWorkflowMetrics : uses_in_scripts
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Cache-backed evaluator with unloadable assemblies modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs, modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/ScriptAssemblyLoadContext.cs |
Implements persistent type cache keyed by SHA-256 hash of code and configuration; adds collectible AssemblyLoadContext for memory-safe assembly unloading. Introduces public cache management methods (ClearCache(), InvalidateScript<T>()), property CachedTypeCount, and supporting helpers (GenerateCacheKey(), CreateDefaultReferences(), CreateDefaultOptions()). Adds new ScriptAssemblyLoadContext class enabling deterministic assembly cleanup. |
Dependency injection and evaluator registration src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cs, src/BBT.Workflow.Application/Scripting/ScriptEngine.cs |
Registers IEvaluator as singleton (CSharpEvaluator) in DI container with updated documentation. Updates ScriptEngine constructor to accept injected IEvaluator instead of hardcoding instantiation; modifies XML documentation. |
Sequence Diagram(s)
sequenceDiagram
actor Client
participant ScriptEngine
participant CSharpEvaluator
participant Cache as Type Cache
participant CompilationSystem as Compilation System
participant SALC as ScriptAssemblyLoadContext
Client->>ScriptEngine: Execute script
ScriptEngine->>CSharpEvaluator: CompileToInstanceAsync<T>(code, ...)
CSharpEvaluator->>CSharpEvaluator: GenerateCacheKey(code, config)
CSharpEvaluator->>Cache: Check for cached type
alt Cache Hit
Cache-->>CSharpEvaluator: Return (ALContext, Type)
CSharpEvaluator-->>CSharpEvaluator: Create instance from cached type
else Cache Miss
CSharpEvaluator->>CSharpEvaluator: CreateDefaultReferences()
CSharpEvaluator->>CompilationSystem: Compile script to assembly
CompilationSystem-->>CSharpEvaluator: Assembly bytecode
CSharpEvaluator->>SALC: new ScriptAssemblyLoadContext(name)
CSharpEvaluator->>SALC: Load compiled assembly (collectible)
SALC-->>CSharpEvaluator: Loaded Assembly
CSharpEvaluator->>CSharpEvaluator: Locate Type implementing T
CSharpEvaluator->>Cache: Store (SALC, Type)
CSharpEvaluator-->>CSharpEvaluator: Create instance from new type
end
CSharpEvaluator-->>ScriptEngine: Instance<T>
ScriptEngine-->>Client: Execution result
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~22 minutes
Areas requiring extra attention:
- Cache key generation logic (
GenerateCacheKey()) — ensure SHA-256 hashing is deterministic and order-insensitive across code, configuration, and directive combinations - AssemblyLoadContext lifecycle management in
CompileAndCacheAsync<T>()— verify proper unload semantics when assembly loading fails or when cached contexts are discarded viaClearCache()/InvalidateScript<T>() - Metadata reference caching (
DefaultMetadataReferences) — confirm Lazy initialization is thread-safe and references are stable across compilations - Dependency injection wiring — ensure singleton
IEvaluatorregistration does not introduce unintended state sharing or lifecycle issues
Poem
🐰 Hop, compile, and cache with care,
Assemblies unload through the air,
Memory-safe scripts now shine so bright,
Collectible contexts make it right!
Dependency injection, clean and spry—
Performance soars, no leaks nearby! ✨
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'fix memory leak issue' is directly related to the main changeset, which introduces memory-safe caching and collectible AssemblyLoadContext for unloading compiled assemblies to prevent memory leaks. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
f/script-memoru-leak-fix
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
Summary of ChangesHello @middt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical memory leak issue within the dynamic C# script evaluation component. By adopting Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
_typeCacheinCSharpEvaluatoris unbounded and only cleared via explicitClearCache/InvalidateScriptcalls; consider adding an internal eviction strategy (e.g., size cap or LRU) to avoid trading one long-lived memory issue for another when many distinct scripts are compiled over time. - Because
DefaultMetadataReferencesis built once from the assemblies loaded at first access, any assemblies loaded later into the AppDomain will not be seen by the evaluator; if late-loaded assemblies are a realistic scenario, you may want a refresh mechanism or a way to inject additional default references.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_typeCache` in `CSharpEvaluator` is unbounded and only cleared via explicit `ClearCache`/`InvalidateScript` calls; consider adding an internal eviction strategy (e.g., size cap or LRU) to avoid trading one long-lived memory issue for another when many distinct scripts are compiled over time.
- Because `DefaultMetadataReferences` is built once from the assemblies loaded at first access, any assemblies loaded later into the AppDomain will not be seen by the evaluator; if late-loaded assemblies are a realistic scenario, you may want a refresh mechanism or a way to inject additional default references.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a memory leak issue by introducing a new CSharpEvaluator that utilizes collectible AssemblyLoadContexts. The implementation includes a robust caching mechanism for compiled scripts, which enhances performance by avoiding redundant compilations. The transition to using dependency injection for the evaluator is a solid design choice that promotes better testability and a cleaner architecture. My review includes a few suggestions to improve code clarity, robustness, and maintainability by refining comments and handling exceptions more explicitly.
| catch | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This catch block is very broad and swallows all exceptions silently, which can hide underlying issues and make debugging difficult. For example, if an assembly file is corrupted or has permission issues, it will fail silently. It's better to catch more specific exceptions that you expect MetadataReference.CreateFromFile to throw (like IOException, NotSupportedException). If logging is not feasible in this static context, consider adding a comment explaining why exceptions are being ignored.
| try | ||
| { | ||
| cached.Context.Unload(); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore unload failures | ||
| } |
There was a problem hiding this comment.
The try-catch block around cached.Context.Unload() is unnecessary. According to the official .NET documentation, the AssemblyLoadContext.Unload() method does not throw any exceptions. You can safely remove this try-catch block to simplify the code.
cached.Context.Unload();| try | ||
| { | ||
| cached.Context.Unload(); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore | ||
| } |
There was a problem hiding this comment.
Similar to the ClearCache method, this try-catch block is unnecessary because AssemblyLoadContext.Unload() does not throw exceptions. Removing it will make the code cleaner.
cached.Context.Unload();| { | ||
| services.TryAddSingleton<IScriptContextFactory, ScriptContextFactory>(); | ||
|
|
||
| // Evaluator is stateless - singleton for efficiency (caches MetadataReferences only) |
There was a problem hiding this comment.
This comment is slightly inaccurate. CSharpEvaluator is not stateless; it maintains a cache of compiled script types in _typeCache. The main reason for its singleton registration is to share this cache across the application. The metadata references are cached in a static field, so they would be shared regardless of the instance's lifetime. A more precise comment would improve clarity.
// The evaluator is thread-safe and manages a cache of compiled scripts.
// A singleton lifetime ensures this cache is shared throughout the application, improving performance.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs (1)
71-142: Critical: Handle cache race condition to prevent context leak.Lines 120-138 create a collectible
AssemblyLoadContextand attempt to cache it. However, ifTryAddat line 138 returnsfalse(another thread cached the same key first), the current thread'sloadContextis neither cached nor unloaded, resulting in a leaked context that cannot be garbage collected.Apply this fix to unload the context if caching fails:
- // Cache the type for future reuse (same script = same assembly = same type) - _typeCache.TryAdd(cacheKey, (loadContext, matchedType)); + // Cache the type for future reuse (same script = same assembly = same type) + if (!_typeCache.TryAdd(cacheKey, (loadContext, matchedType))) + { + // Another thread cached this script first - unload our context and use theirs + try + { + loadContext.Unload(); + } + catch + { + // Ignore unload failures + } + + // Get the cached version that won the race + var cached = _typeCache[cacheKey]; + return Task.FromResult((T)Activator.CreateInstance(cached.CompiledType)!); + }
🧹 Nitpick comments (2)
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs (2)
209-225: Document blocking behavior and concurrency semantics.The implementation is thread-safe for concurrent calls, but
AssemblyLoadContext.Unload()may block if assemblies are still referenced. Consider documenting:
- This method may block until assemblies can be unloaded
- New compilations can occur concurrently with clearing
- Clearing is best-effort—active scripts may prevent immediate unload
Consider adding XML documentation:
/// <summary> /// Clears all cached types and unloads their assemblies. /// Call this to reclaim memory if script definitions change. +/// Note: This method may block if cached assemblies are still in use. +/// Concurrent compilations are allowed and will add new cache entries. /// </summary> public void ClearCache()
231-252: LGTM! Clean targeted invalidation API.The implementation correctly invalidates specific scripts by matching the cache key generation logic. The generic parameter
<T>is necessary since the same script code targeting different types produces different cache entries.Consider exposing
InvalidateScript<T>andClearCacheon theIEvaluatorinterface to enable cache management in tests and support cache eviction policies without depending on the concrete implementation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs(3 hunks)modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/ScriptAssemblyLoadContext.cs(1 hunks)src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cs(2 hunks)src/BBT.Workflow.Application/Scripting/ScriptEngine.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/vnext.mdc)
**/*.cs: Write concise, idiomatic C# code with accurate examples
Follow Aether Framework's recommended folder and module structure (*.Application, *.Domain, *.Infrastructure, *.HttpApi)
Use object-oriented and functional programming patterns as appropriate
Prefer LINQ and lambda expressions for collection operations
Use descriptive variable and method names (e.g., IsUserSignedIn, CalculateTotal)
Follow Microsoft's modular development approach with extension structure to separate concerns between layers (Application, Domain, Infrastructure, etc.)
Organize extensions following the namespace of the class or structure where the extension will be written, generally in BBT.Workflow.Domain project to minimize 'using' statements
Follow Clean Architecture and SOLID principles
Apply Domain-Driven Design patterns: Aggregates, Entities, ValueObjects, Repositories, Domain Events
All cross-cutting concerns such as Clock, GuidGenerator, Mapper, Tracing, Logging, Metrics MUST use Aether SDK
Workflow, orchestration, task handling and runtime logic MUST follow vNext architectural conventions
Always use async/await
Use Dependency Injection everywhere
Avoid leaking EF entities
Apply Result Pattern
Use PascalCase for class names, method names, and public members
Use camelCase for local variables and private fields
Use UPPERCASE for constants
Prefix interface names with 'I' (e.g., IUserService)
Use C# 10+ features when appropriate (e.g., record types, pattern matching, null-coalescing assignment)
Leverage built-in ASP.NET Core features and middleware, as well as Aether's modules and features
Use Entity Framework Core effectively for database operations, integrating with Aether's AetherDbContext and repository abstractions
Follow the C# Coding Conventions from Microsoft documentation
Use C#'s expressive syntax (e.g., null-conditional operators, string interpolation)
Use 'var' for implicit typing when the type is obvious
Use exceptions for exceptional cases, not for control flow
Implemen...
Files:
src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cssrc/BBT.Workflow.Application/Scripting/ScriptEngine.csmodules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.csmodules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/ScriptAssemblyLoadContext.cs
🧠 Learnings (10)
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Use BackgroundService for task processors and IHostedService for lifecycle control
Applied to files:
src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Implement background tasks using IHostedService or BackgroundService
Applied to files:
src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Organize extensions following the namespace of the class or structure where the extension will be written, generally in BBT.Workflow.Domain project to minimize 'using' statements
Applied to files:
src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Use Dependency Injection everywhere
Applied to files:
src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Always use IDistributedCache for caching strategies instead of IMemoryCache
Applied to files:
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Use asynchronous programming with async/await for I/O-bound operations
Applied to files:
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Use C# 10+ features when appropriate (e.g., record types, pattern matching, null-coalescing assignment)
Applied to files:
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Leverage built-in ASP.NET Core features and middleware, as well as Aether's modules and features
Applied to files:
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Use C#'s expressive syntax (e.g., null-conditional operators, string interpolation)
Applied to files:
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs
📚 Learning: 2025-12-15T22:59:58.153Z
Learnt from: CR
Repo: burgan-tech/vnext PR: 0
File: .cursor/rules/vnext.mdc:0-0
Timestamp: 2025-12-15T22:59:58.153Z
Learning: Applies to **/*.cs : Write concise, idiomatic C# code with accurate examples
Applied to files:
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs
🧬 Code graph analysis (2)
src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cs (2)
src/BBT.Workflow.Domain/Scripting/Factory/Services/ScriptContextFactory.cs (1)
ScriptContextFactory(11-23)modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs (1)
CSharpEvaluator(24-262)
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs (3)
src/BBT.Workflow.Application/Scripting/ScriptEngine.cs (1)
Task(82-162)modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/IEvaluator.cs (1)
Task(10-14)modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/ScriptAssemblyLoadContext.cs (2)
ScriptAssemblyLoadContext(11-31)ScriptAssemblyLoadContext(17-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Sourcery review
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and analyze
🔇 Additional comments (7)
src/BBT.Workflow.Application/Microsoft/Extensions/DependencyInjection/TaskServiceCollectionExtensions.cs (2)
2-2: LGTM! Clear documentation of memory management strategy.The added documentation accurately describes how the collectible AssemblyLoadContext prevents memory leaks from dynamic script compilation by enabling garbage collection of assemblies when no longer referenced.
Also applies to: 158-159
164-167: Verify caching approach against coding guidelines.The singleton registration is appropriate for a stateless evaluator. However, the comment "caches MetadataReferences only" is incomplete—the evaluator also caches compiled Types in an in-memory ConcurrentDictionary (see
CSharpEvaluator._typeCacheat line 30).Per coding guidelines: "Always use IDistributedCache for caching strategies instead of IMemoryCache." The current implementation uses in-memory caching, which may not be suitable in distributed/multi-instance scenarios where cache invalidation and sharing would be beneficial.
Consider whether:
- Script cache should be shared across multiple application instances via IDistributedCache
- The current in-memory approach is acceptable given that compiled assemblies cannot be serialized and distributed
- Cache eviction/invalidation strategies are sufficient for long-running processes
Based on coding guidelines, the in-memory cache approach should be justified if it deviates from the IDistributedCache requirement.
src/BBT.Workflow.Application/Scripting/ScriptEngine.cs (1)
17-27: LGTM! Proper dependency injection pattern.The refactor from hardcoded
new CSharpEvaluator()to constructor injection follows the "Use Dependency Injection everywhere" coding guideline and improves testability and lifecycle management.modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/ScriptAssemblyLoadContext.cs (1)
1-31: LGTM! Clean and correct collectible context implementation.The implementation is minimal and correct:
- The
isCollectible: trueflag enables assembly unloading, which is the core mechanism for preventing memory leaks- Returning
nullfromLoaddelegates to default resolution, allowing scripts to reuse assemblies from the main application without duplication- The unique name generation with Guid aids in debugging and diagnostics
modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.cs (3)
19-42: LGTM! Thread-safe cache implementation with proper context tracking.The implementation correctly uses:
ConcurrentDictionaryfor thread-safe cache operationsLazy<T>withExecutionAndPublicationmode for thread-safe initialization of default references- Tuple storage of
(AssemblyLoadContext, Type)to enable proper unloadingThe
CachedTypeCountproperty provides useful observability for monitoring cache growth.
147-179: LGTM! Robust cache key generation with proper collision resistance.The implementation correctly produces stable, unique cache keys by:
- Using
AssemblyQualifiedNameto distinguish types including version info- Sorting collections with
OrderByto ensure order independence- Using SHA-256 for strong collision resistance
The use of
MetadataReference.Display(typically file paths) should be stable across executions given consistent assembly locations.
185-203: LGTM! Defensive assembly probing with proper error handling.The implementation correctly:
- Filters dynamic assemblies that cannot have metadata references
- Uses try-catch to handle inaccessible assemblies gracefully
- Leverages lazy initialization to compute references once
The empty catch block at line 195 is acceptable for this probing scenario where continuing despite individual failures is the desired behavior.
| public Task<T> CompileToInstanceAsync<T>( | ||
| string code, | ||
| IEnumerable<MetadataReference>? extraReferences = null, | ||
| IEnumerable<string>? usingDirectives = null, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(code)) | ||
| throw new ArgumentException("Code cannot be null or empty", nameof(code)); | ||
|
|
||
| // Generate cache key from code + target type + configuration | ||
| var cacheKey = GenerateCacheKey(code, typeof(T), extraReferences, usingDirectives); | ||
|
|
||
| // Try to get cached type and create instance from it | ||
| if (_typeCache.TryGetValue(cacheKey, out var cached)) | ||
| { | ||
| var instance = (T)Activator.CreateInstance(cached.CompiledType)!; | ||
| return Task.FromResult(instance); | ||
| } | ||
|
|
||
| // Compile, cache the type, and return instance | ||
| return CompileAndCacheAsync<T>(code, cacheKey, extraReferences, usingDirectives, cancellationToken); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the CSharpEvaluator file and related interface files
find . -name "*Evaluator*" -type f | head -20
# Find the IEvaluator interface
find . -name "*IEvaluator*" -type f
# Search for CompileAndCacheAsync method
rg -l "CompileAndCacheAsync" --type=csRepository: burgan-tech/vnext
Length of output: 963
🏁 Script executed:
#!/bin/bash
# Read the IEvaluator interface to see if it documents constructor requirements
cat -n ./modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/IEvaluator.cs
echo "---"
# Read the CSharpEvaluator file to examine implementation details
wc -l ./modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.csRepository: burgan-tech/vnext
Length of output: 696
🏁 Script executed:
#!/bin/bash
# Read the entire CSharpEvaluator file to understand the implementation
cat -n ./modules/BBT.Workflow.Modules.Scripting/BBT/Workflow/Scripting/Evaluators/CSharpEvaluator.csRepository: burgan-tech/vnext
Length of output: 11093
🏁 Script executed:
#!/bin/bash
# Search for tests related to CSharpEvaluator to understand expected behavior
find . -name "*Test*" -o -name "*test*" | grep -i evaluator
# Search for any usage of CompileToInstanceAsync in the codebase
rg "CompileToInstanceAsync" --type=cs -B 2 -A 5
# Search for Activator.CreateInstance patterns in the codebase
rg "Activator\.CreateInstance" --type=cs -B 2 -A 2Repository: burgan-tech/vnext
Length of output: 40513
🏁 Script executed:
#!/bin/bash
# Check if there are any tests specifically for constructor validation
rg "MissingMethodException" --type=cs
# Check test implementations to see if they ever test with parameterized constructors
rg "public class.*:" test/BBT.Workflow.Application.Tests/Scripting/ --type=cs -A 3 | head -40
# Look for any try-catch around Activator.CreateInstance patterns
rg "try.*Activator\.CreateInstance|catch.*MissingMethodException" --type=cs -B 2 -A 3Repository: burgan-tech/vnext
Length of output: 3645
🏁 Script executed:
#!/bin/bash
# Look at ScriptValidationTests to see what validation is performed
cat -n test/BBT.Workflow.Application.Tests/Scripting/ScriptValidationTests.cs | head -100
echo "---"
# Look for any test cases that might test with a constructor that has parameters
rg "public class.*\(" test/BBT.Workflow.Application.Tests/Scripting/ --type=cs -A 1Repository: burgan-tech/vnext
Length of output: 4162
🏁 Script executed:
#!/bin/bash
# Check all test classes to see if any test with parameterized constructors
find test/BBT.Workflow.Application.Tests/Scripting -name "*.cs" -exec grep -l "public.*(" {} \;
# Look at a specific test to see the pattern of classes being compiled
cat -n test/BBT.Workflow.Application.Tests/Scripting/ScriptEngineTests.cs | head -150Repository: burgan-tech/vnext
Length of output: 6817
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that explicitly test error handling for constructor issues
rg "MissingMethodException|parameterless|constructor" test/BBT.Workflow.Application.Tests/Scripting/ --type=cs
# Check if there's any exception handling around the instance creation
cat -n src/BBT.Workflow.Application/Scripting/ScriptEngine.cs | head -100Repository: burgan-tech/vnext
Length of output: 5543
🏁 Script executed:
#!/bin/bash
# Read the rest of ScriptEngine.cs to see exception handling
cat -n src/BBT.Workflow.Application/Scripting/ScriptEngine.cs | tail -100Repository: burgan-tech/vnext
Length of output: 5279
🏁 Script executed:
#!/bin/bash
# Final verification: Check if MissingMethodException is handled anywhere in the call chain
rg "MissingMethodException|catch.*Exception.*Activator" --type=cs
# Verify the exact behavior of Activator.CreateInstance by looking at any documentation
rg "Activator\.CreateInstance.*System.Reflection" --type=csRepository: burgan-tech/vnext
Length of output: 43
Document and validate parameterless constructor requirement for compiled types.
Line 60 uses Activator.CreateInstance(cached.CompiledType) which requires a parameterless constructor. If compiled code defines a type with only parameterized constructors, this throws MissingMethodException at runtime—caught only as a generic exception, not a domain-specific error.
Add:
- XML documentation to
CompileToInstanceAsync<T>specifying that compiled types must have a parameterless constructor - Validation during compilation to verify the matched type has a parameterless constructor (inspect via reflection)
- Catch and wrap
MissingMethodExceptionwith a clear error message indicating the requirement
This ensures developers get actionable feedback instead of cryptic reflection errors.
3 participants
Summary by Sourcery
Introduce a memory-safe C# script evaluator that uses collectible assembly load contexts and shared caching to prevent memory leaks from dynamic script compilation, and wire it into the application’s scripting services.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.