Skip to content

Conversation

@schomatis
Copy link
Contributor

No description provided.

handler.go Outdated
// Limit request size. Ideally this limit should be specific for each field
// in the JSON request but as simple defensive measure we just limit the
// entire HTTP body.
MAX_REQUEST_SIZE := 10 << 10 // FIXME: Agree on a value and extract.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some Worker calls to things which compute snarks send ~20-30MB, so I'd probably set this to 100M

Choose a reason for hiding this comment

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

@magik6k are there any cases where 100MB+ would happen on requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also fine with this being a configurable option (with a sane default like 100 MiB) for the consumer to adjust as necessary. Not sure what would be the most appropriate way to implement that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it as a server configuration, will make Lotus consume it as a node options during dependency update.

@Kubuxu
Copy link
Contributor

Kubuxu commented Nov 12, 2020

Q: Why do we read it into a buffer instead of decoding it directly from the request reader?

@schomatis
Copy link
Contributor Author

Q: Why do we read it into a buffer instead of decoding it directly from the request reader?

To be able to tell if the client sent more than the limit and report it back as an explicit error instead of just silently truncating it with the limited reader and report a parsing error. Open to other alternatives on how to implement it, though, see the FIXMEs.

@schomatis schomatis self-assigned this Nov 13, 2020
@Kubuxu
Copy link
Contributor

Kubuxu commented Nov 13, 2020

Ahh, ignore my question, we were using raw decoder previously.

@schomatis schomatis marked this pull request as ready for review November 16, 2020 17:13
@magik6k magik6k merged commit f4b2d34 into master Nov 16, 2020
@magik6k magik6k deleted the schomatis/fix/limit-request-size branch November 16, 2020 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants