Веб-сервер: прием WebSocket и повторный Запустить by sfaqer · Pull Request #1778 · EvilBeaver/OneScript

sfaqer · 2026-10-01T00:14:29Z

ВебСокет.ПолучитьСтроку() и ПолучитьДвоичныеДанные() писали в результат весь буфер на 1024 байта, а не result.Count, и возвращали GetBuffer() — в конце любого сообщения был мусор/нули. Теперь только полученные байты.

Прием сообщений идет под блокировкой: WebSocket допускает одно ожидающее получение, и два задания, читающие один сокет, падали с InvalidOperationException; сообщение из нескольких частей читает один поток целиком.

Повторный ВебСервер.Запустить() (например, из фонового задания) подменял приложение: первый запуск потом освобождал чужое, а Остановить() первый запуск не останавливал. Теперь это ошибка «Веб-сервер уже запущен». Заодно обертки свойств контекста запроса создаются под блокировкой — контекст могут передать в задания.

Тесты WebServerConcurrencyTests в Core.Tests.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- WebSocket messages are now received as complete, separate messages when multiple reads occur concurrently.
- Starting a server that is already running now returns a clear error. The server can be started again after it stops or encounters a failure.
- Improved reliability when multiple operations access shared server data.

coderabbitai · 2026-10-01T00:14:37Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 54cf4330-055d-419b-b87f-ba3d9de93265

📥 Commits

Reviewing files that changed from the base of the PR and between 36e2ca9 and bfcc2c5.

📒 Files selected for processing (4)

src/OneScript.Web.Server/PropertyWrappersCollection.cs
src/OneScript.Web.Server/WebServer.cs
src/OneScript.Web.Server/WebSockets/WebSocketWrapper.cs
src/Tests/OneScript.Core.Tests/WebServerConcurrencyTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Walkthrough

The changes synchronize property-wrapper cache access, prevent overlapping server runs, and serialize WebSocket receive operations. Integration tests cover concurrent socket reads and attempts to start the same server twice.

Changes: Web server concurrency

Layer / File(s)	Summary
Property-wrapper cache access `src/OneScript.Web.Server/PropertyWrappersCollection.cs`	`Get<T>` now locks the cache during lookup, wrapper creation, insertion, and return.
Server run guard `src/OneScript.Web.Server/WebServer.cs`	`Run` rejects a start while another run is active and resets the running flag after completion or failure. The app reference is now volatile.
Complete WebSocket receives `src/OneScript.Web.Server/WebSockets/WebSocketWrapper.cs`	Receive operations now use a shared lock. Message fragments append only the bytes reported as received, and string and binary methods use the accumulated message bytes.
Concurrency integration tests `src/Tests/OneScript.Core.Tests/WebServerConcurrencyTests.cs`	Tests exercise concurrent reads on one socket and rejection of a second server start. Helpers set up the engine and exchange WebSocket frames.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to bfcc2

The changes protect concurrent server operations and return correctly sized WebSocket messages. The tests reject padded message results; no actionable merge-blocking issue was identified.

Security architecture risk: 🔵 Low · up to bfcc2

The changes improve concurrent server operation and message integrity. No introduced security-boundary expansion was established. Assurance remains limited for interrupted receives and concurrent shutdown or restart behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — A peer withholding the remainder of a message can keep a high-level receiver waiting and queue other receivers behind it on the same wrapper. The added monitor is wrapper-local, not a service-wide lock. Aggregate resource exposure depends on application task creation and connection limits, which were not established.

Security Findings and Attack Paths

observed — Unbounded fragmented-message accumulation and receives without an explicit cancellation token existed in the base implementation. The head retains these limits while correcting byte-count handling and serialization; the comparison does not establish a newly introduced resource-exhaustion vulnerability.

Trust Boundaries and Controls

observed — The inspected script-visible acceptance path wraps the socket returned by AcceptWebSocketAsync in a new WebSocketWrapper. Synchronization is therefore scoped to that wrapper; the public C# constructor does not itself enforce unique wrapping of an underlying socket.
inferred — The lock prevents simultaneous underlying receives but does not assign logical message ownership across successive low-level Receive calls. A low-level call returning an incomplete fragment can be followed by a high-level call consuming the remainder. This possibility predates the PR and still requires caller coordination.

Resilience and Maintainability Implications

inferred — Receive exceptions release the monitor, and Abort, Close and CloseOutput do not acquire it, so the new lock does not prevent entry to those terminal operations. Completion of concurrent close handshakes depends on the underlying socket implementation and was not verified.

Hardening Proposals

proposed — Consider explicit message-size and receive-duration limits, with cancellation and cleanup semantics, to bound the inherited exposure to peers that send oversized or unfinished messages.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the two main changes: WebSocket message reception and repeated web-server startup handling.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

sfaqer · 2026-10-01T00:43:17Z

@coderabbitai review

coderabbitai · 2026-10-01T00:43:25Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

sfaqer · 2026-10-01T00:59:16Z

@coderabbitai review

coderabbitai · 2026-10-01T00:59:24Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

sfaqer · 2026-10-01T01:44:35Z

@coderabbitai review

coderabbitai · 2026-10-01T01:44:43Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

# Conflicts: # src/OneScript.Web.Server/WebServer.cs

EvilBeaver · 2026-10-15T13:32:57Z

            finally
            {
-                _app.DisposeAsync().AsTask().Wait();
+                System.Threading.Volatile.Write(ref _isRunning, 0);


А не проще было бы volatile bool поле объявить, чем вот эту конструкцию использовать?

volatile дает только видимость, а тут надо проверить и выставить одним действием: два Запустить из разных потоков оба прочитали бы Ложь и оба запустили бы сервер. Могу переписать на lock с обычным bool, если так понятнее.

ПолучитьСтроку и ПолучитьДвоичныеДанные писали весь буфер на 1024 байта и возвращали внутренний массив потока - в конце сообщения был мусор. Прием сообщений идет под блокировкой: два задания на одном сокете падали с InvalidOperationException. Повторный Запустить того же сервера подменял приложение, и Остановить не останавливал первый запуск - теперь это ошибка. Обертки свойств контекста запроса создаются под блокировкой. Co-Authored-By: Claude Opus 5.5 <[email protected]>

sonar-openbsl-ru-qa-bot · 2026-10-23T14:02:59Z

Quality Gate passed

Issues

Measures

Project ID: OneScript

View in SonarQube

sfaqer force-pushed the bugfix/web-server-thread-safety branch from bfcc2c5 to cb5e7f8 Compare October 2, 2026 00:23

sfaqer added a commit to sfaqer/OneScript that referenced this pull request Oct 2, 2026

Merge PR EvilBeaver#1778 (bugfix/web-server-thread-safety)

95df395

# Conflicts: # src/OneScript.Web.Server/WebServer.cs

sfaqer force-pushed the bugfix/web-server-thread-safety branch from cb5e7f8 to dc9bd18 Compare October 15, 2026 12:17

EvilBeaver reviewed Oct 15, 2026

View reviewed changes

sfaqer force-pushed the bugfix/web-server-thread-safety branch from dc9bd18 to 14fb34a Compare October 23, 2026 13:56

Navigation Menu

Repository navigation

Веб-сервер: прием WebSocket и повторный Запустить - #1778

Веб-сервер: прием WebSocket и повторный Запустить#1778
sfaqer wants to merge 1 commit into
EvilBeaver:developfrom
sfaqer:bugfix/web-server-thread-safety

sfaqer commented Oct 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 1, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

sfaqer commented Oct 1, 2026

Uh oh!

coderabbitai Bot commented Oct 1, 2026 •

edited

Loading

Uh oh!

sfaqer commented Oct 1, 2026

Uh oh!

coderabbitai Bot commented Oct 1, 2026 •

edited

Loading

Uh oh!

sfaqer commented Oct 1, 2026

Uh oh!

coderabbitai Bot commented Oct 1, 2026 •

edited

Loading

Uh oh!

EvilBeaver Oct 15, 2026

Uh oh!

sfaqer Oct 15, 2026

Uh oh!

sonar-openbsl-ru-qa-bot Bot commented Oct 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sfaqer commented Oct 1, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Failed checks (1 warning)

Uh oh!

sfaqer commented Oct 1, 2026

Uh oh!

coderabbitai Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfaqer commented Oct 1, 2026

Uh oh!

coderabbitai Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfaqer commented Oct 1, 2026

Uh oh!

coderabbitai Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EvilBeaver Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

sfaqer Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

sonar-openbsl-ru-qa-bot Bot commented Oct 23, 2026

Quality Gate passed

Issues

Measures

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sfaqer commented Oct 1, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 1, 2026 •

edited

Loading

coderabbitai Bot commented Oct 1, 2026 •

edited

Loading

coderabbitai Bot commented Oct 1, 2026 •

edited

Loading

coderabbitai Bot commented Oct 1, 2026 •

edited

Loading