feat(binary): лимит памяти для ДвоичныеДанные из конфигурации (#1667)#1670
Open
johnnyshut wants to merge 5 commits into
Open
feat(binary): лимит памяти для ДвоичныеДанные из конфигурации (#1667)#1670johnnyshut wants to merge 5 commits into
johnnyshut wants to merge 5 commits into
Conversation
Добавили ключ binaryData.inMemoryMaxBytes (байты) в oscript.cfg и переменную OSCRIPT_CONFIG. Изменили OneScriptCoreOptions: разбор значения с проверкой и запасным значением по умолчанию (50 МБ). Добавили BinaryDataConfigurationDefaults, IBinaryDataMemoryLimit и регистрацию в SetDefaultOptions. Добавили BinaryDataRuntimeSettings (AsyncLocal): на время BslProcess.Run лимит берётся из DI; вне процесса — дефолт. Изменили BinaryDataContext и конструктор FileBackingStream без параметров для использования эффективного лимита. Изменили FileBackingConstants.DEFAULT_MEMORY_LIMIT — ссылка на общий дефолт из ScriptEngine. Изменили CGI: WebRequestContext принимает лимит из сервисов до CreateProcess.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a OneScript.BinaryData project with config defaults, parsing, DI contract and implementation, and runtime settings; replaces hardcoded 50MiB thresholds with the runtime limit in binary components and web POST buffering; wires DI and startup application; updates solution configs; and hardens executor lifecycle in BslProcess.Run. ChangesBinary Data Memory Limit Configuration
Process Executor Lifecycle Management
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/ScriptEngine/Machine/BinaryDataRuntimeSettings.cs (1)
20-24: ⚡ Quick winHarden runtime settings against invalid DI-provided limits
PushFromServicescurrently trustsIBinaryDataMemoryLimit.MaxBytesInMemory. A custom host registration can pass invalid values and break downstream buffering behavior; sanitize here before storing toAsyncLocal.💡 Suggested fix
internal static void PushFromServices(IServiceContainer services) { var opts = services.TryResolve<IBinaryDataMemoryLimit>(); - InMemoryMaxBytes.Value = opts?.MaxBytesInMemory ?? BinaryDataConfigurationDefaults.InMemoryMaxBytes; + var maxBytes = opts?.MaxBytesInMemory ?? BinaryDataConfigurationDefaults.InMemoryMaxBytes; + if (maxBytes <= 0 || maxBytes == int.MaxValue) + maxBytes = BinaryDataConfigurationDefaults.InMemoryMaxBytes; + + InMemoryMaxBytes.Value = maxBytes; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScriptEngine/Machine/BinaryDataRuntimeSettings.cs` around lines 20 - 24, PushFromServices currently writes IBinaryDataMemoryLimit.MaxBytesInMemory directly into the AsyncLocal InMemoryMaxBytes.Value; validate and sanitize the DI-provided value first so invalid or negative numbers don't corrupt runtime buffering. In PushFromServices, read opts?.MaxBytesInMemory into a local, check for null/<=0 and replace with BinaryDataConfigurationDefaults.InMemoryMaxBytes, and optionally clamp extremely large values to a sane upper bound before assigning to InMemoryMaxBytes.Value; reference IBinaryDataMemoryLimit, PushFromServices, InMemoryMaxBytes and BinaryDataConfigurationDefaults.InMemoryMaxBytes when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/oscript/CgiBehavior.cs`:
- Around line 72-73: Replace the hard Resolve call so CGI startup uses a safe DI
lookup with a default fallback: try to get IBinaryDataMemoryLimit from
engine.Services (avoid engine.Services.Resolve which throws) and if null or
missing use BinaryDataConfigurationDefaults.InMemoryMaxBytes, then pass that
value into the WebRequestContext constructor; update the code locations
referencing IBinaryDataMemoryLimit, engine.Services.Resolve and the
WebRequestContext creation to use this conditional/fallback logic.
In `@src/oscript/Web/WebRequestContext.cs`:
- Around line 37-40: Validate the constructor parameter in WebRequestContext by
checking postBodyInMemoryMaxBytes at the start of the WebRequestContext(int
postBodyInMemoryMaxBytes) constructor and throw an ArgumentOutOfRangeException
(with a clear message mentioning _postBodyInMemoryMaxBytes and expected positive
byte count) when the value is invalid (e.g., <= 0 or otherwise out of allowed
range) so the error fails fast instead of propagating later into
FileBackingStream.
- Line 79: The code constructs FileBackingStream with the full content length
which causes MemoryStream to preallocate huge buffers; change the capacity
argument so initial allocation is capped by _postBodyInMemoryMaxBytes (e.g.,
pass Math.Min(len, _postBodyInMemoryMaxBytes) instead of len) when creating the
FileBackingStream (the call that assigns dest = new FileBackingStream(...)) so
the in-memory buffer is limited and spilling-to-disk behavior works as intended.
In `@src/ScriptEngine/BslProcess.cs`:
- Around line 45-46: Wrap the runtime push/pop and _isRunning mutation in a
try/finally so Pop() and _isRunning reset always run even if executor hooks
throw: call BinaryDataRuntimeSettings.PushFromServices and set _isRunning = true
before invoking Array.ForEach(_executorProviders, e =>
e.BeforeProcessStart(this)), then ensure BinaryDataRuntimeSettings.Pop() and
_isRunning = false are executed in a finally block; similarly, ensure any
Array.ForEach(_executorProviders, e => e.AfterProcessExit(this)) failures do not
prevent the finally from running (catch per-hook exceptions if desired) so Pop()
and _isRunning reset cannot be skipped.
---
Nitpick comments:
In `@src/ScriptEngine/Machine/BinaryDataRuntimeSettings.cs`:
- Around line 20-24: PushFromServices currently writes
IBinaryDataMemoryLimit.MaxBytesInMemory directly into the AsyncLocal
InMemoryMaxBytes.Value; validate and sanitize the DI-provided value first so
invalid or negative numbers don't corrupt runtime buffering. In
PushFromServices, read opts?.MaxBytesInMemory into a local, check for null/<=0
and replace with BinaryDataConfigurationDefaults.InMemoryMaxBytes, and
optionally clamp extremely large values to a sane upper bound before assigning
to InMemoryMaxBytes.Value; reference IBinaryDataMemoryLimit, PushFromServices,
InMemoryMaxBytes and BinaryDataConfigurationDefaults.InMemoryMaxBytes when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22f9a33d-f478-4b86-9fcf-a6bc6faba769
📒 Files selected for processing (12)
src/OneScript.StandardLibrary/Binary/BinaryDataContext.cssrc/OneScript.StandardLibrary/Binary/FileBackingConstants.cssrc/OneScript.StandardLibrary/Binary/FileBackingStream.cssrc/ScriptEngine.HostedScript/oscript.cfgsrc/ScriptEngine/BinaryDataConfigurationDefaults.cssrc/ScriptEngine/BslProcess.cssrc/ScriptEngine/Hosting/EngineBuilderExtensions.cssrc/ScriptEngine/IBinaryDataMemoryLimit.cssrc/ScriptEngine/Machine/BinaryDataRuntimeSettings.cssrc/ScriptEngine/OneScriptCoreOptions.cssrc/oscript/CgiBehavior.cssrc/oscript/Web/WebRequestContext.cs
… обработки ошибок Логика уведомлений исполнителя перемещена в единый условный блок для большей ясности. Улучшена обработка ошибок при завершении процесса для обеспечения надлежащей очистки ресурсов. Обновлены параметры BinaryDataRuntimeSettings для обеспечения положительных ограничений памяти и резервных значений по умолчанию. Скорректирован WebRequestContext для проверки ограничений памяти при создании экземпляра.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/oscript/Web/WebRequestContext.cs (1)
37-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
Int32.MaxValueat the constructor boundary too.
FileBackingStreamtreatsInt32.MaxValueasSYSTEM_IN_MEMORY_LIMITand throws, soWebRequestContext(int)still accepts a value it cannot actually use. Failing fast here keeps the public API consistent and avoids a later exception fromProcessPostData().Suggested fix
- if (postBodyInMemoryMaxBytes <= 0) + if (postBodyInMemoryMaxBytes <= 0 || postBodyInMemoryMaxBytes == int.MaxValue) throw new ArgumentOutOfRangeException(nameof(postBodyInMemoryMaxBytes), - "Лимит памяти для тела POST-запроса должен быть положительным числом байт."); + "Лимит памяти для тела POST-запроса должен быть положительным числом байт и меньше Int32.MaxValue.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/oscript/Web/WebRequestContext.cs` around lines 37 - 42, The WebRequestContext constructor currently only rejects non-positive values but must also reject Int32.MaxValue because FileBackingStream treats that value as SYSTEM_IN_MEMORY_LIMIT and will throw later; update the WebRequestContext(int postBodyInMemoryMaxBytes) constructor to validate that postBodyInMemoryMaxBytes is > 0 and < Int32.MaxValue (or != Int32.MaxValue) and throw an ArgumentOutOfRangeException with a clear message if the value is Int32.MaxValue so callers fail fast (this keeps behavior consistent with FileBackingStream and avoids a later exception in ProcessPostData()).src/ScriptEngine/BslProcess.cs (1)
49-68:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUnwind executors that already started when a later
BeforeProcessStartthrows.
_isRunningbecomestrueonly after the whole startup loop succeeds, so a failure in any laterBeforeProcessStartskipsAfterProcessExitfor executors that already initialized state.StackMachineExecutor.BeforeProcessStart()allocates machine/debugger state (src/ScriptEngine/Machine/StackMachineExecutor.cs:25-45), so failed startup can still leak per-process setup here.Suggested direction
+ var startedExecutors = new List<IExecutorProvider>(); try { if (notifyExecutors) - Array.ForEach(_executorProviders, e => e.BeforeProcessStart(this)); + { + foreach (var executor in _executorProviders) + { + executor.BeforeProcessStart(this); + startedExecutors.Add(executor); + } + } _isRunning = true; return _bslExecutorsByModule[module.GetType()](this, target, module, method, arguments); } finally { if (notifyExecutors) { try { - if (_isRunning) - Array.ForEach(_executorProviders, e => e.AfterProcessExit(this)); + for (var i = startedExecutors.Count - 1; i >= 0; i--) + startedExecutors[i].AfterProcessExit(this); } finally { BinaryDataRuntimeSettings.Pop(); _isRunning = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScriptEngine/BslProcess.cs` around lines 49 - 68, When calling each executor's BeforeProcessStart (via Array.ForEach over _executorProviders) a later exception can skip AfterProcessExit for earlier executors; modify BslProcess.Start (the block invoking BeforeProcessStart/AfterProcessExit) to track which providers successfully completed BeforeProcessStart (e.g., a local list/flag per provider) and set _isRunning only for the process-level state, then in the finally ensure you call AfterProcessExit for every provider that was marked started (not gated solely on _isRunning), and still call BinaryDataRuntimeSettings.Pop() and clear _isRunning; in short: record started executors during the BeforeProcessStart loop and unwind them via their AfterProcessExit even if startup throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/oscript/Web/WebRequestContext.cs`:
- Around line 37-42: The WebRequestContext constructor currently only rejects
non-positive values but must also reject Int32.MaxValue because
FileBackingStream treats that value as SYSTEM_IN_MEMORY_LIMIT and will throw
later; update the WebRequestContext(int postBodyInMemoryMaxBytes) constructor to
validate that postBodyInMemoryMaxBytes is > 0 and < Int32.MaxValue (or !=
Int32.MaxValue) and throw an ArgumentOutOfRangeException with a clear message if
the value is Int32.MaxValue so callers fail fast (this keeps behavior consistent
with FileBackingStream and avoids a later exception in ProcessPostData()).
In `@src/ScriptEngine/BslProcess.cs`:
- Around line 49-68: When calling each executor's BeforeProcessStart (via
Array.ForEach over _executorProviders) a later exception can skip
AfterProcessExit for earlier executors; modify BslProcess.Start (the block
invoking BeforeProcessStart/AfterProcessExit) to track which providers
successfully completed BeforeProcessStart (e.g., a local list/flag per provider)
and set _isRunning only for the process-level state, then in the finally ensure
you call AfterProcessExit for every provider that was marked started (not gated
solely on _isRunning), and still call BinaryDataRuntimeSettings.Pop() and clear
_isRunning; in short: record started executors during the BeforeProcessStart
loop and unwind them via their AfterProcessExit even if startup throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67fd241a-6dc6-485e-962b-7b19be63e644
📒 Files selected for processing (4)
src/ScriptEngine/BslProcess.cssrc/ScriptEngine/Machine/BinaryDataRuntimeSettings.cssrc/oscript/CgiBehavior.cssrc/oscript/Web/WebRequestContext.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ScriptEngine/Machine/BinaryDataRuntimeSettings.cs
dmpas
reviewed
May 14, 2026
EvilBeaver
requested changes
May 15, 2026
…ичных данных Созданы новые классы и интерфейсы для управления лимитом памяти двоичных данных, включая BinaryDataConfigurationDefaults, IBinaryDataMemoryLimit и BinaryDataMemoryLimit. Реализована регистрация лимита памяти в EngineBuilderExtensions и обновлены соответствующие классы для использования нового механизма. Удален устаревший BinaryDataRuntimeSettings. Обновлены тестовые и основные приложения для интеграции новых настроек.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ScriptEngine/BslProcess.cs (1)
64-75: 💤 Low valueConsider wrapping each
AfterProcessExitcall to ensure all executors receive cleanup notification.If one executor's
AfterProcessExitthrows, the loop exits and remaining executors won't be notified. Depending on what executors do (e.g., release resources, pop state), this could leave other executors in an inconsistent state.If best-effort cleanup for all executors is desired:
♻️ Optional: catch-and-aggregate exceptions per executor
if (startedExecutors != null) { + List<Exception> cleanupExceptions = null; for (var i = startedExecutors.Count - 1; i >= 0; i--) - startedExecutors[i].AfterProcessExit(this); + { + try + { + startedExecutors[i].AfterProcessExit(this); + } + catch (Exception ex) + { + cleanupExceptions ??= new List<Exception>(); + cleanupExceptions.Add(ex); + } + } + if (cleanupExceptions != null) + throw new AggregateException(cleanupExceptions); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ScriptEngine/BslProcess.cs` around lines 64 - 75, The loop calling startedExecutors[i].AfterProcessExit(this) can abort if one executor throws; modify the loop in BslProcess (where startedExecutors and _isRunning are used) to call each executor inside its own try/catch so all executors get notified even if one fails, and optionally collect/aggregate exceptions (e.g., into a List<Exception>) to rethrow or log after the loop; preserve the existing finally that sets _isRunning = false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/ScriptEngine/BslProcess.cs`:
- Around line 64-75: The loop calling startedExecutors[i].AfterProcessExit(this)
can abort if one executor throws; modify the loop in BslProcess (where
startedExecutors and _isRunning are used) to call each executor inside its own
try/catch so all executors get notified even if one fails, and optionally
collect/aggregate exceptions (e.g., into a List<Exception>) to rethrow or log
after the loop; preserve the existing finally that sets _isRunning = false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0f91c0b-43c7-4ece-9f63-ae751783b71c
📒 Files selected for processing (19)
src/1Script.slnsrc/OneScript.BinaryData/BinaryDataConfigurationDefaults.cssrc/OneScript.BinaryData/BinaryDataMemoryLimit.cssrc/OneScript.BinaryData/BinaryDataMemoryLimitConfiguration.cssrc/OneScript.BinaryData/BinaryDataRuntimeSettings.cssrc/OneScript.BinaryData/IBinaryDataMemoryLimit.cssrc/OneScript.BinaryData/OneScript.BinaryData.csprojsrc/OneScript.StandardLibrary/Binary/BinaryDataContext.cssrc/OneScript.StandardLibrary/Binary/FileBackingConstants.cssrc/OneScript.StandardLibrary/Binary/FileBackingStream.cssrc/OneScript.StandardLibrary/EngineBuilderExtensions.cssrc/ScriptEngine/BslProcess.cssrc/ScriptEngine/Hosting/EngineBuilderExtensions.cssrc/ScriptEngine/ScriptEngine.csprojsrc/ScriptEngine/ScriptingEngine.cssrc/TestApp/MainWindow.xaml.cssrc/oscript/CgiBehavior.cssrc/oscript/ConsoleHostBuilder.cssrc/oscript/Web/WebRequestContext.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/OneScript.StandardLibrary/Binary/BinaryDataContext.cs
- src/ScriptEngine/Hosting/EngineBuilderExtensions.cs
- src/oscript/Web/WebRequestContext.cs
- src/OneScript.StandardLibrary/Binary/FileBackingStream.cs
…для лимита памяти двоичных данных
Contributor
Author
|
@EvilBeaver исправил замечания |
EvilBeaver
reviewed
May 20, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OneScript.BinaryData/BinaryDataMemoryLimitConfiguration.cs`:
- Around line 83-85: The multiplication of number * multiplier can overflow
before the post-check; update the logic in BinaryDataMemoryLimitConfiguration
(the code using variables number, multiplier and result) to pre-guard the
multiplication by checking bounds (e.g., if multiplier <= 0 or number <= 0
return false, and return false if number > int.MaxValue / multiplier or if
number > long.MaxValue / multiplier as appropriate), or perform the
multiplication inside a checked block and catch OverflowException, then only
assign to result when it is guaranteed within int.MaxValue; ensure the final
check still validates result <= 0 || result >= int.MaxValue as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff309b71-5a27-4cf1-ba0a-b4b1fcddf4ff
📒 Files selected for processing (6)
src/OneScript.BinaryData/BinaryDataConfigurationDefaults.cssrc/OneScript.BinaryData/BinaryDataMemoryLimitConfiguration.cssrc/OneScript.StandardLibrary/EngineBuilderExtensions.cssrc/ScriptEngine.HostedScript/oscript.cfgsrc/Tests/OneScript.StandardLibrary.Tests/BinaryDataMemoryLimitConfigurationTests.cssrc/Tests/OneScript.StandardLibrary.Tests/OneScript.StandardLibrary.Tests.csproj
EvilBeaver
reviewed
May 20, 2026
| @@ -0,0 +1,16 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
Owner
There was a problem hiding this comment.
Еще один проект?Почему? Я может что-то упускаю или недопонимаю? Почему вся эта кухня не может просто жить внутри StandardLibrary, ведь она единственный ее потребитель?
Contributor
Author
There was a problem hiding this comment.
Используют ScriptEngine (ApplyFromServices при старте) и StandardLibrary. Могу в StandardLibrary перенести
Owner
There was a problem hiding this comment.
Дай мне, пожалуйста, доступ в свой форк на запись, я попробую написать, что я имею в виду
Contributor
Author
There was a problem hiding this comment.
https://github.com/johnnyshut/OneScript
Проверь, пожалуйста, добавил
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
3 participants
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.
Зачем
Закрывает #1667: порог «держать двоичные данные в памяти до перехода на временный файл» больше не зашит константой только в коде — задаётся через конфигурацию.
Что сделали
binaryData.inMemoryMaxBytes(целое число байт) вoscript.cfgи в состав настроек изOSCRIPT_CONFIG. Если ключ не задан или значение некорректно — используется 50 МБ (BinaryDataConfigurationDefaults.InMemoryMaxBytes).IBinaryDataMemoryLimitи регистрацию вSetDefaultOptions(singleton на основеOneScriptCoreOptions).BinaryDataRuntimeSettings: на время выполненияBslProcess.Runлимит попадает вAsyncLocalи используетсяBinaryDataContextи параметрlessFileBackingStream(); вне процесса BSL остаётся значение по умолчанию.FileBackingConstants.DEFAULT_MEMORY_LIMIT— тот же дефолт, что и вScriptEngine.WebRequestContextполучает лимит из контейнера после сборки движка (доCreateProcess), т.к. запрос создаётся раньше старта BSL-процесса.Как проверить
oscript.cfgраскомментировать/задать, например:binaryData.inMemoryMaxBytes=1048576, убедиться, что поведение буферизации соответствует ожиданиям для больших потоков.dotnet build src/oscript/oscript.csproj(как минимум).Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores