Использование потока httpResponse без вычитывания по возможности#1618
Использование потока httpResponse без вычитывания по возможности#1618EvilBeaver merged 4 commits intoEvilBeaver:developfrom
Conversation
WalkthroughHttp response body storage and read behavior were changed: body may be file-backed, in-memory, or served directly from the raw response stream. HttpResponseContext gained a dumpToFile-aware constructor, stores the raw response and adapts headers when auto-decompressing. New tests add SSE and binary-response scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HttpResponseContext
participant HttpResponseBody
participant Stream as "File / Memory / RawStream"
Client->>HttpResponseContext: new(response, dumpToFile)
HttpResponseContext->>HttpResponseBody: new(response, dumpToFile, headers, AutoDecompress)
Note over HttpResponseBody: Decide storage: file-backed, in-memory, or none
HttpResponseContext->>HttpResponseContext: store raw response stream (_rawStream)
Client->>HttpResponseContext: GetBodyAsStream()/GetBodyAsBinaryData()/GetBodyAsString()
HttpResponseContext->>HttpResponseBody: OpenReadStream(_rawStream)
alt Backing file exists
HttpResponseBody-->>Stream: return FileStream
else In-memory body exists
HttpResponseBody-->>Stream: return MemoryStream
else
HttpResponseBody-->>Stream: return provided rawStream (fallback)
end
Stream-->>HttpResponseContext: stream data
HttpResponseContext-->>Client: result (stream/string/binary)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (1)
54-69: Remove the unusedRetrieveResponseDatamethod at lines 54-69.The method is dead code with no callers and duplicates logic from the constructor. Ripgrep confirms zero call sites. The private method is unused and safe to delete.
🧹 Nitpick comments (4)
src/OneScript.StandardLibrary/Http/HttpResponseBody.cs (1)
75-87: Исправьте несогласованные отступы и рассмотрите проверку на null.Код имеет несогласованные отступы (3 пробела вместо 4). Также, если
rawStreamравенnullи нет ни файла, ни буфера в памяти, метод вернётnull, что может привести кNullReferenceExceptionу вызывающего кода.public Stream OpenReadStream(Stream rawStream) { if (_backingFileName != null) { return new FileStream(_backingFileName, FileMode.Open, FileAccess.Read); } - else if (_inMemBody != null) - { - return new MemoryStream(_inMemBody); - } + else if (_inMemBody != null) + { + return new MemoryStream(_inMemBody); + } else - return rawStream; + return rawStream ?? throw new InvalidOperationException("Response body is not available"); }tests/http.os (2)
394-435: Рассмотрите добавление обработки исключений для очистки ресурсов.Тест проверяет важный сценарий потоковой передачи SSE, но если тест упадёт до строки 434, веб-сервер останется запущенным в фоновом задании. Рекомендуется обернуть тест в
Попытка...Исключениедля гарантированной очистки.Процедура ТестДолженПроверитьПолучениеStreamEvent() Экспорт МенеджерФоновыхЗаданий = Новый МенеджерФоновыхЗаданий; ЗаданиеВебсервера = МенеджерФоновыхЗаданий.Выполнить(ЭтотОбъект, "Вебсервер"); Попытка // ... основной код теста ... Исключение МенеджерФоновыхЗаданий.Очистить(); ВызватьИсключение; КонецПопытки; МенеджерФоновыхЗаданий.Очистить(); КонецПроцедуры
399-399: Рассмотрите возможность использования динамического порта.Порт 8081 захардкожен, что может привести к конфликтам при параллельном запуске тестов или если порт занят другим приложением.
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (1)
191-198: Исправьте несогласованные отступы в методе Dispose.Строки 194 и 197 имеют некорректные отступы.
public void Dispose() { _response?.Dispose(); - _response = null; - + _response = null; + _body?.Dispose(); - _body = null; + _body = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/OneScript.StandardLibrary/Http/HttpResponseBody.cs(3 hunks)src/OneScript.StandardLibrary/Http/HttpResponseContext.cs(6 hunks)tests/http.os(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sfaqer
Repo: EvilBeaver/OneScript PR: 0
File: :0-0
Timestamp: 2025-05-23T03:00:18.462Z
Learning: User sfaqer prefers communication in Russian language.
🔇 Additional comments (4)
src/OneScript.StandardLibrary/Http/HttpResponseBody.cs (1)
40-47: Изменена логика инициализации тела ответа.Теперь тело ответа инициализируется только если указан файл для сохранения или требуется автоматическая распаковка gzip. Это позволяет использовать поток напрямую для потоковой передачи данных (SSE).
tests/http.os (1)
382-392: Хороший тест для проверки получения двоичных данных.Тест корректно проверяет получение двоичных данных из HTTP-ответа и валидирует размер полученных данных.
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (2)
31-32: Добавлены поля для хранения сырого потока и ответа.Поля
_rawStreamи_responseнеобходимы для поддержки потоковой передачи данных без буферизации.
37-52: Корректная реализация конструктора для потоковой передачи.Конструктор правильно организует порядок операций: сначала обрабатывает тело ответа (что может или не может потребить поток), затем сохраняет ссылку на поток для случая потоковой передачи.
|
Предыдущий PR закрыть получается? |
Да пусть повисит в драфте. Там работает вроде и рисков на все разломать сильно меньше |
ПР два. Но делают они совершенно разное. Какой вливать решать тебе =) Первый устроит меня по функциональности но вносит несовместимость с 1с по методам HTTPСоединения. Второй меняет поведение у текущей реализации HTTPЗапроса так, что фиг знает где оно стрельнет. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (2)
38-38: Fix inconsistent indentation.Line 38 has an extra leading space compared to other lines in the constructor.
- StatusCode = (int)response.StatusCode; + StatusCode = (int)response.StatusCode;
189-196: Fix inconsistent indentation in Dispose method.Lines 192 and 195 have extra indentation that breaks the visual consistency.
public void Dispose() { _response?.Dispose(); - _response = null; - + _response = null; + _body?.Dispose(); - _body = null; + _body = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Http/HttpResponseBody.cs(4 hunks)src/OneScript.StandardLibrary/Http/HttpResponseContext.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/OneScript.StandardLibrary/Http/HttpResponseBody.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
Learnt from: sfaqer
Repo: EvilBeaver/OneScript PR: 0
File: :0-0
Timestamp: 2025-05-23T03:00:18.462Z
Learning: User sfaqer prefers communication in Russian language.
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (2)
src/OneScript.StandardLibrary/Http/HttpResponseBody.cs (2)
Dispose(196-205)Dispose(225-228)src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (1)
Dispose(139-142)
🔇 Additional comments (4)
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (4)
31-32: LGTM - Field addition for response lifecycle management.The
_responsefield is necessary to keep the response alive for streaming scenarios, aligning with the PR objective to avoid reading the entire response upfront.
146-151: LGTM - Stream copy pattern for unknown-length streams.The change from direct
stream.Lengthaccess toMemoryStream.CopyTois necessary because raw network streams may not support seeking or length queries. This aligns with the PR's goal of using response streams directly.Note: For very large responses,
ToArray()creates an additional memory copy. Consider pre-sizing the MemoryStream whenContentLengthis known to reduce reallocations:using (var stream = _body.OpenReadStream()) -using (var memoryStream = new MemoryStream()) +using (var memoryStream = _body.ContentSize > 0 + ? new MemoryStream((int)Math.Min(_body.ContentSize, int.MaxValue)) + : new MemoryStream()) { stream.CopyTo(memoryStream); return new BinaryDataContext(memoryStream.ToArray()); }
52-67:RetrieveResponseDataappears to be dead code.This method duplicates the logic now in the constructor but uses the old
using(response)pattern that disposes the response immediately. After the constructor refactoring, this method seems unused.#!/bin/bash # Search for usages of RetrieveResponseData in the codebase rg -n "RetrieveResponseData" --type cs
158-165: Verify stream reusability for multipleGetBody*calls.
GetBodyAsStreamreturns aGenericStreamwithownsStream=true, meaning the caller can dispose the underlying stream. After this, subsequent calls toGetBodyAsString,GetBodyAsBinaryData, orGetBodyAsStreammay fail if the raw stream is not seekable or is already consumed.This could be a behavioral change if callers previously expected to call multiple
GetBody*methods on the same response object.
|
|
||
| Вебсервер = Новый ВебСервер(ПортТестовогоСервера); | ||
| Вебсервер.ДобавитьОбработчикЗапросов(ЭтотОбъект, "ОбработчикЗапроса"); | ||
| Вебсервер.Запустить(); |
There was a problem hiding this comment.
А как гасить? Процессы тестов не виснут?
2 participants
Использование потока httpResponse без вычитывания по возможности
Для unzip и сохранения в файл оставил то что в body, в надежде не поломать совместимость.
issues: #1607
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.