Conversation
WalkthroughHTTP method invocation refactored across two context classes to support streaming responses. The Changes
Sequence DiagramsequenceDiagram
participant Caller
participant HttpConnCtx as HttpConnectionContext
participant HttpRespCtx as HttpResponseContext
participant WebResponse as HttpWebResponse
rect rgb(200, 220, 255)
Note over Caller,WebResponse: Normal (Buffered) Response Path
Caller->>HttpConnCtx: CallHTTPMethod(method, request, output)
HttpConnCtx->>HttpConnCtx: GetResponse(..., rawStream=false)
HttpConnCtx->>HttpRespCtx: new(..., rawStream=false)
HttpRespCtx->>HttpRespCtx: RetrieveResponseData() - buffer body
HttpConnCtx-->>Caller: HttpResponseContext (body buffered)
end
rect rgb(200, 255, 220)
Note over Caller,WebResponse: Streaming Response Path
Caller->>HttpConnCtx: CallHTTPMethodStream(method, request)
HttpConnCtx->>HttpConnCtx: GetResponse(..., rawStream=true)
HttpConnCtx->>HttpRespCtx: new(..., rawStream=true)
HttpRespCtx->>HttpRespCtx: Store response stream directly
HttpConnCtx-->>Caller: HttpResponseContext (stream stored)
end
rect rgb(255, 240, 200)
Note over Caller,HttpRespCtx: GetBodyAsStream() - returns raw stream when available
Caller->>HttpRespCtx: GetBodyAsStream()
alt rawStream available
HttpRespCtx-->>Caller: _rawStream
else buffered body
HttpRespCtx-->>Caller: body as stream
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (2)
115-136: Несогласованность API: метод не работает с сырыми потоками.Когда
rawStream=true, поле_bodyравноnull, но_rawStreamустановлен. Проверка на строке 117 вернетНеопределено, хотя данные доступны через_rawStream. Это создает несогласованность:GetBodyAsStream()работает с сырыми потоками, аGetBodyAsString()— нет.Рассмотрите возможность чтения из
_rawStream, если он доступен:public IValue GetBodyAsString(IValue encoding = null) { + if (_rawStream != null) + { + Encoding enc; + if (encoding == null) + { + if (String.IsNullOrEmpty(_defaultCharset)) + _defaultCharset = "utf-8"; + enc = Encoding.GetEncoding(_defaultCharset); + } + else + enc = TextEncodingEnum.GetEncoding(encoding); + + using(var reader = new StreamReader(_rawStream, enc)) + { + return ValueFactory.Create(reader.ReadToEnd()); + } + } + if (_body == null) return ValueFactory.Create();Примечание: Это сделает поток непригодным для повторного использования, если он не поддерживает поиск.
143-154: Несогласованность API: метод не работает с сырыми потоками.Аналогично
GetBodyAsString(), этот метод не может обработать случай, когда установлен_rawStream, но_bodyравенnull.Рассмотрите возможность чтения из
_rawStream:public IValue GetBodyAsBinaryData() { + if (_rawStream != null) + { + using (var ms = new MemoryStream()) + { + _rawStream.CopyTo(ms); + return new BinaryDataContext(ms.ToArray()); + } + } + if (_body == null) return ValueFactory.Create();Примечание: Это материализует весь поток в память, что может противоречить цели использования сырого потока для больших ответов.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs(4 hunks)src/OneScript.StandardLibrary/Http/HttpResponseContext.cs(3 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.
🧬 Code graph analysis (2)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (2)
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (6)
HttpResponseContext(36-52)ContextMethod(114-136)ContextMethod(142-154)ContextMethod(160-170)ContextMethod(176-183)ContextMethod(188-192)src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (1)
HttpRequestContext(28-32)
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (2)
src/OneScript.StandardLibrary/Binary/GenericStream.cs (4)
Stream(18-18)Stream(282-285)GenericStream(37-42)GenericStream(44-49)src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (1)
HttpResponseContext(350-378)
🔇 Additional comments (5)
src/OneScript.StandardLibrary/Http/HttpResponseContext.cs (2)
31-31: Field addition looks good.The
_rawStreamfield appropriately stores the raw response stream for streaming scenarios.
163-164: Проверьте поведение при повторном вызове.Метод возвращает тот же экземпляр
_rawStreamпри каждом вызове. Если поток не поддерживает поиск (неSeekable), повторное чтение невозможно, и второй вызовGetBodyAsStream()вернет уже прочитанный поток.Убедитесь, что это задокументировано или рассмотрите возможность проверки, был ли поток уже прочитан.
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (3)
201-205: AI summary inconsistency detected.The AI summary incorrectly states that
Patchwas renamed toCallHTTPMethod. However, thePatchmethod still exists unchanged at line 178.CallHTTPMethodis a new method that provides a generic way to call arbitrary HTTP methods, whilePatchremains as a convenience method specifically for PATCH requests. Both methods coexist and serve different purposes.
201-205: LGTM! Generic HTTP method invocation added.The implementation correctly provides a way to invoke arbitrary HTTP methods, following the same pattern as the existing verb-specific methods (Get, Post, etc.). The delegation to
GetResponseis appropriate.
350-350: LGTM! Clean parameter threading for streaming support.The changes correctly thread the
rawStreamparameter through the request processing pipeline:
- Default value
falsemaintains backward compatibility with existing callers- Parameter properly propagated to
HttpResponseContextconstructor- Minimal, focused changes that don't affect existing functionality
Also applies to: 374-374
| /// <summary> | ||
| /// Вызвать произвольный HTTP-метод с потоком ответа | ||
| /// </summary> | ||
| /// <param name="method">Строка. Имя метода HTTP</param> | ||
| /// <param name="request">HTTPЗапрос. Данные и заголовки запроса http</param> | ||
| /// <returns>HTTPОтвет. Ответ сервера.</returns> | ||
| [ContextMethod("ВызватьHTTPМетодПоток", "CallHTTPMethodStream")] | ||
| public HttpResponseContext CallHTTPMethodStream(string method, HttpRequestContext request) | ||
| { | ||
| return GetResponse(request, method, null, true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify HttpWebResponse disposal in streaming scenarios.
The streaming implementation looks correct and follows appropriate patterns. The method properly delegates to GetResponse with rawStream=true and hardcodes output to null (which is appropriate for streaming).
However, please verify that HttpWebResponse resources are properly managed when streaming. In the current implementation:
- The response object (line 364) is passed to
HttpResponseContextbut not explicitly stored or disposed - The stream keeps the response alive, but if callers don't properly dispose the stream returned by
GetBodyAsStream(), connections may leak - Streaming scenarios can hold connections longer, making resource leaks more impactful
Consider verifying:
- That
GenericStreamproperly disposes the underlying stream (thetrueparameter at HttpResponseContext line 164 suggests it does) - Whether
HttpResponseContextshould hold a reference to the response and implementIDisposableto ensure cleanup - Adding usage documentation about the importance of disposing the stream
🏁 Script executed:
#!/bin/bash
# Verify GenericStream disposal behavior and IDisposable implementation
# Check if GenericStream properly implements disposal
ast-grep --pattern 'class GenericStream $$$'
# Check if HttpResponseContext implements IDisposable
rg -n "class HttpResponseContext" -A 5Length of output: 105523
---
🏁 Script executed:
```shell
#!/bin/bash
# Check HttpResponseContext Dispose implementation and GetResponse method
echo "=== HttpResponseContext Dispose Implementation ==="
grep -n "Dispose\|GetResponse" src/OneScript.StandardLibrary/Http/HttpResponseContext.cs | head -20
echo ""
echo "=== Looking for GetResponse method in HttpConnectionContext ==="
grep -n -A 15 "GetResponse" src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs | head -40
Length of output: 2006
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the private GetResponse method implementation
echo "=== GetResponse private method in HttpConnectionContext ==="
grep -n -A 25 "private.*GetResponse" src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs | head -50
echo ""
echo "=== HttpResponseBody class definition and Dispose ==="
grep -n -B 2 -A 15 "class HttpResponseBody" src/OneScript.StandardLibrary/Http/HttpResponseBody.cs
Length of output: 1861
---
🏁 Script executed:
```shell
#!/bin/bash
# Find HttpResponseContext constructor
echo "=== HttpResponseContext Constructor ==="
grep -n -A 20 "public HttpResponseContext" src/OneScript.StandardLibrary/Http/HttpResponseContext.cs | head -30
echo ""
echo "=== HttpResponseBody full Dispose implementation ==="
grep -n -A 25 "public void Dispose" src/OneScript.StandardLibrary/Http/HttpResponseBody.cs
echo ""
echo "=== Check if response is stored in HttpResponseContext ==="
grep -n "HttpWebResponse\|_response" src/OneScript.StandardLibrary/Http/HttpResponseContext.cs
Length of output: 1327
---
🏁 Script executed:
```shell
#!/bin/bash
# Check _rawStream field and complete Dispose implementation
echo "=== HttpResponseContext fields and full Dispose ==="
grep -n "_rawStream\|private\|public void Dispose" src/OneScript.StandardLibrary/Http/HttpResponseContext.cs | head -20
echo ""
echo "=== Full HttpResponseContext.Dispose and related code ==="
sed -n '190,210p' src/OneScript.StandardLibrary/Http/HttpResponseContext.cs
echo ""
echo "=== HttpResponseBody Dispose(bool) implementation ==="
sed -n '208,232p' src/OneScript.StandardLibrary/Http/HttpResponseBody.cs
Length of output: 1530
Store and dispose HttpWebResponse in streaming scenarios to prevent connection leaks.
The streaming implementation has a critical resource leak. In the CallHTTPMethodStream code path:
- The
HttpWebResponseobject created at line 364 inGetResponse()is passed toHttpResponseContextbut never stored or disposed HttpResponseContext.Dispose()only disposes_body, ignoring both_rawStreamand the response object- In contrast, the non-streaming path correctly uses
using(response)to dispose the response - Unreferenced
HttpWebResponseobjects leak, exhausting the connection pool—particularly problematic in streaming scenarios where connections are held longer
Fix: Store the HttpWebResponse as a private field in HttpResponseContext and dispose it in the Dispose() method, similar to how the non-streaming path handles it with using().
| public HttpResponseContext(HttpWebResponse response, string dumpToFile, bool rawStream) | ||
| { | ||
| RetrieveResponseData(response, null); | ||
| } | ||
|
|
||
| public HttpResponseContext(HttpWebResponse response, string dumpToFile) | ||
| { | ||
| RetrieveResponseData(response, dumpToFile); | ||
| if (!rawStream) | ||
| { | ||
|
|
||
| RetrieveResponseData(response, dumpToFile); | ||
| } | ||
| else | ||
| { | ||
| StatusCode = (int)response.StatusCode; | ||
| _defaultCharset = response.CharacterSet; | ||
|
|
||
| ProcessHeaders(response.Headers); | ||
| _rawStream = response.GetResponseStream(); | ||
| } | ||
| } |
There was a problem hiding this comment.
КРИТИЧЕСКАЯ ПРОБЛЕМА: Утечка ресурсов HTTP-ответа.
В режиме rawStream=true объект response не освобождается. В отличие от RetrieveResponseData (строка 56), который использует using(response), путь обработки сырого потока (строки 44-51) не вызывает Dispose() для объекта HttpWebResponse. Это приведет к утечке соединений, памяти и дескрипторов.
Примените этот diff для сохранения ссылки на response и его освобождения:
+ private HttpWebResponse _response;
+
public HttpResponseContext(HttpWebResponse response, string dumpToFile, bool rawStream)
{
if (!rawStream)
{
RetrieveResponseData(response, dumpToFile);
}
else
{
StatusCode = (int)response.StatusCode;
_defaultCharset = response.CharacterSet;
ProcessHeaders(response.Headers);
_rawStream = response.GetResponseStream();
+ _response = response;
}
}Затем обновите метод Dispose():
public void Dispose()
{
+ if (_response != null)
+ {
+ _response.Dispose();
+ _response = null;
+ }
+
if (_body != null)
+ {
_body.Dispose();
+ _body = null;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/OneScript.StandardLibrary/Http/HttpResponseContext.cs around lines 36 to
52, the rawStream branch stores the response stream but never retains or
disposes the HttpWebResponse causing resource leaks; modify the constructor to
save the passed HttpWebResponse to a private field (e.g. _response) when
rawStream is true instead of only calling GetResponseStream(), and ensure
ProcessHeaders/StatusCode/_defaultCharset remain set; then update the class
Dispose() to check and call Dispose() (or Close()) on the stored _response (and
null it) in addition to disposing the response stream so the HttpWebResponse is
always released.
| } | ||
|
|
||
| private HttpResponseContext GetResponse(HttpRequestContext request, string method, string output = null) | ||
| private HttpResponseContext GetResponse(HttpRequestContext request, string method, string output = null, bool rawStream = false) |
There was a problem hiding this comment.
А если просто без дополнительных параметров и методов - отдавать сырой поток в методе ПолучитьТелоКакПоток? так не получается?
There was a problem hiding this comment.
Я не разобрался зачем он вообще вчитывается в методах заполнения тела =(
There was a problem hiding this comment.
Может попробовать сделать все-таки? Мне кажется, там не обязательно трогать сетевой поток, если не вызывались методы ПолучитьТелоКакСтроку/ДвоичныеДанные
There was a problem hiding this comment.
Попробовать могу, но точно что то отломиться т.к. сейчас поток для чего то вычитывается в боди...
|
Закрою в пользу #1618 |
2 participants
Добавлен метод для работы с сырым потоком HTTPОтвета
issues: #1607
Summary by CodeRabbit
New Features
Refactor