-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Oracle limit #800
Add Oracle limit #800
Conversation
@dotnet-policy-service agree
|
@roman-khimov Please, could you take a look? |
Is the response size of NeoFS also limited? |
It should be limited by the client, but currently it's not. So the problem exists for The neofs-api-csharp Client is capable to handle such large "virtual" objects and doesn't restrict their maximum size: neo-modules/src/OracleService/Protocols/OracleNeoFSProtocol.cs Lines 97 to 101 in 4e31068
and neo-modules/src/OracleService/Protocols/OracleNeoFSProtocol.cs Lines 103 to 108 in 4e31068
Preferred solutionIdeally, to solve this problem we need a change in the neofs-api-csharp Client's API to limit the amount of data that can be read from the stream. In neofs-sdk-go it is done by returning the Another possible solutionAnother way to solve this problem without neofs-api-csharp Client's API changes is to use GetObjectPayloadRangeData combined with manually pre-checked range lenght on the |
Co-authored-by: Erik Zhang <erik@neo.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also consider fixing OracleNeoFSProtocol wrt #800 (comment).
private readonly HttpClient client = new(new HttpClientHandler() { AllowAutoRedirect = false }); | ||
private readonly HttpClient client = new(new HttpClientHandler() { AllowAutoRedirect = false }) | ||
{ | ||
MaxResponseContentBufferSize = OracleResponse.MaxResultSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably adjust the exception handler of OracleService
wrt this change. Currently if the responce content exceeds OracleResponse.MaxResultSize
length, the exception will be thrown by message.Content.ReadAsStringAsync(...)
method:
return (OracleResponseCode.Success, await message.Content.ReadAsStringAsync(cancellation)); |
It will be catched by the following
catch
block:neo-modules/src/OracleService/OracleService.cs
Lines 361 to 368 in 4e31068
try | |
{ | |
return await protocol.ProcessAsync(uri, ctsLinked.Token); | |
} | |
catch (Exception ex) | |
{ | |
return (OracleResponseCode.Error, $"Request <{url}> Error:{ex.Message}"); | |
} |
As a result, we'll have
OracleResponseCode.Error
code instead of OracleResponseCode.ResponseTooLarge
which is more appropriate in this case. In neo-go we return OracleResponseCode.ResponseTooLarge
in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which exception is throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to you to investigate which one. The documentation doesn't name it: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.maxresponsecontentbuffersize?view=net-8.0#remarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@superboyiii could you test it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@superboyiii could you test it?
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@superboyiii could you test it?
@shargon Due to my test result, the same as @AnnaShaleva said. We need solve it.
@AnnaShaleva @shargon There is a stream version GetObject here https://github.com/neo-ngd/neofs-api-csharp/blob/82ca3de1d87c96a04bdf094c60c7c6039bbacf81/src/Neo.FileStorage.API/client/Client.Object.cs#LL39C9-L39C9 |
Co-authored-by: Owen Zhang <38493437+superboyiii@users.noreply.github.com>
@@ -82,6 +82,8 @@ public async Task<(OracleResponseCode, string)> ProcessAsync(Uri uri, Cancellati | |||
return (OracleResponseCode.Error, message.StatusCode.ToString()); | |||
if (!Settings.Default.AllowedContentTypes.Contains(message.Content.Headers.ContentType.MediaType)) | |||
return (OracleResponseCode.ContentTypeNotSupported, null); | |||
if (!message.Content.Headers.ContentLength.HasValue || message.Content.Headers.ContentLength > OracleResponse.MaxResultSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The !message.Content.Headers.ContentLength.HasValue
part of it may affect legitimate responses that fit the limit, but don't have the header for whatever reason (dynamically generated content, for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can require this header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd be an artificial limitation to me. It shouldn't be hard to handle !message.Content.Headers.ContentLength.HasValue
case as well, NeoGo handles both cases easily.
…-modules into add-oracle-limit
byte[] buffer = new byte[OracleResponse.MaxResultSize]; | ||
var stream = message.Content.ReadAsStream(cancellation); | ||
var read = await stream.ReadAsync(buffer, 0, buffer.Length, cancellation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If response stream contains more than buffer.Length
bytes, then it's an error and we need to return OracleResponseCode.ResponseTooLarge
code. And if I'm not mistaken, the current code will just stop reading and return a part of the response with OracleResponseCode.Success
code (which isn't the desired behaviour).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, check the way how it's done in neo-go: https://github.com/nspcc-dev/neo-go/blob/7b86a54fc0dd918a2514b0108be986a6e2cc63fa/pkg/services/oracle/response.go#L71-L82
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, check it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part LGTM, resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shargon, could you also consider fixing the same problem for OracleNeoFSProtocol as @ZhangTao1596 suggested? #800 (comment)
Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@superboyiii could you test it? |
I'm ing... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request: https://api.github.com/users?since=800&per_page=1000
Works as expected!
No description provided.