feat(binary): лимит памяти для ДвоичныеДанные из конфигурации (#1667)#1670
Open
johnnyshut wants to merge 2 commits intoEvilBeaver:developfrom
Open
feat(binary): лимит памяти для ДвоичныеДанные из конфигурации (#1667)#1670johnnyshut wants to merge 2 commits intoEvilBeaver:developfrom
johnnyshut wants to merge 2 commits intoEvilBeaver:developfrom
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
📝 WalkthroughWalkthroughImplements configurable binary-data in-memory limit: adds configuration/defaults, DI contract, async-local runtime settings pushed per-process, and updates binary buffering and web request code to use the runtime-effective limit instead of a fixed constant. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant DI as Dependency Injection
participant BslProc as BslProcess
participant RTSettings as BinaryDataRuntimeSettings
participant BinOps as Binary Ops (BinaryDataContext / FileBackingStream)
Config->>DI: Register IBinaryDataMemoryLimit (from config)
BslProc->>DI: Start execution / obtain services
BslProc->>RTSettings: PushFromServices(serviceContainer)
RTSettings->>DI: Resolve IBinaryDataMemoryLimit
RTSettings->>RTSettings: Store value in AsyncLocal
BslProc->>BinOps: Perform binary operation (stream/read)
BinOps->>RTSettings: GetEffectiveInMemoryMaxBytes()
RTSettings-->>BinOps: Return configured limit
BinOps->>BinOps: Choose LoadToBuffer or BackingTemporaryFile
BslProc->>RTSettings: Pop()
RTSettings->>RTSettings: Clear AsyncLocal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
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
1 participant
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
Refactor