Skip to content
This repository was archived by the owner on Nov 10, 2025. It is now read-only.

Oracle: neofs result size limit#803

Merged
shargon merged 1 commit intoneo-project:add-oracle-limitfrom
ZhangTao1596:add-fs-limit
Jun 2, 2023
Merged

Oracle: neofs result size limit#803
shargon merged 1 commit intoneo-project:add-oracle-limitfrom
ZhangTao1596:add-fs-limit

Conversation

@ZhangTao1596
Copy link
Collaborator

@ZhangTao1596 ZhangTao1596 commented May 29, 2023

@ZhangTao1596 ZhangTao1596 requested a review from shargon May 29, 2023 08:05
AnnaShaleva
AnnaShaleva reviewed May 29, 2023
View reviewed changes
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this should solve the OOM problem. Could we test this code with some large object or at least add a unit-test?

@AnnaShaleva AnnaShaleva mentioned this pull request May 29, 2023
Open
AnnaShaleva
AnnaShaleva reviewed May 29, 2023
View reviewed changes
src/OracleService/Protocols/OracleNeoFSProtocol.cs
"header" => GetHeaderAsync(client, objectAddr, tokenSource.Token),
"hash" => GetHashAsync(client, objectAddr, ps.Skip(3).ToArray(), tokenSource.Token),
"range" => await GetRangeAsync(client, objectAddr, ps.Skip(3).ToArray(), tokenSource.Token),
"header" => (OracleResponseCode.Success, await GetHeaderAsync(client, objectAddr, tokenSource.Token)),
Copy link
Member

@AnnaShaleva AnnaShaleva May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the record: while GetPayload and GetRangeAsync methods are fixed, the same problem exists with GetHeader. Unfortunately, this problem exists on the NeoFS-API level (the object header size isn't restricted at the API lievel) and can't be easily fixed in the OracleNeoFSProtocol for now. We'll track this issue in nspcc-dev/neofs-api#171.

Copy link
Member

@AnnaShaleva AnnaShaleva May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetHashAsync is also affected, because it also uses client.GetObjectHeader under the hood.

@AnnaShaleva AnnaShaleva mentioned this pull request May 29, 2023
Closed
@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented May 30, 2023

LGTM, this should solve the OOM problem. Could we test this code with some large object or at least add a unit-test?

I will test and add ut.

shargon
shargon reviewed May 30, 2023
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, waiting for the UT

@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented Jun 1, 2023

I test locally with fs object in testnet.
With large object
image
With small object
image

@shargon I have no idea how to add ut. All classes and methods in oracle service are private or protect. May I modify their access modifiers?

@shargon
Copy link
Member

shargon commented Jun 1, 2023

@ZhangTao1596 please accept the license, and we can merge it

@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented Jun 2, 2023

@ZhangTao1596 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@shargon shargon merged commit b7839fc into neo-project:add-oracle-limit Jun 2, 2023
@ZhangTao1596 ZhangTao1596 deleted the add-fs-limit branch June 9, 2023 08:22
superboyiii added a commit that referenced this pull request Jul 12, 2023
@shargon @erikzhang
@superboyiii @AnnaShaleva
Add Oracle limit (#800)
99ffc84 
* Add oracle limit

* AnnaShaleva review

* Update src/OracleService/Protocols/OracleHttpsProtocol.cs

Co-authored-by: Erik Zhang <erik@neo.org>

* Add Response length

* Update src/OracleService/Protocols/OracleHttpsProtocol.cs

Co-authored-by: Owen Zhang <38493437+superboyiii@users.noreply.github.com>

* New version
Extracted from: https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System/net/System/Net/Http/HttpContent.cs#L91

* Fix

* Clean

* Read one more

* oracle: neofs result size limit (#803)

* Enforce UTF8

* Update src/OracleService/Protocols/OracleHttpsProtocol.cs

Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>

---------

Co-authored-by: Erik Zhang <erik@neo.org>
Co-authored-by: Owen Zhang <38493437+superboyiii@users.noreply.github.com>
Co-authored-by: ZhangTao <zhangtao@ngd.neo.org>
Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@shargon shargon shargon approved these changes

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ZhangTao1596 @shargon @AnnaShaleva