Skip to content
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

limit response for HTTP adapters to 32kb #1278

Merged
merged 5 commits into from
May 29, 2019
Merged

Conversation

dimroc
Copy link
Contributor

@dimroc dimroc commented May 28, 2019

Note that this implementation ignores http header Content-Length because a malicious user could lie, and we would have to count the bytes of the payload anyways.

@dimroc dimroc force-pushed the chores/limit-http-get branch from 75560ae to 23ab08c Compare May 28, 2019 22:16
@dimroc dimroc marked this pull request as ready for review May 28, 2019 22:32
// too (it returns (0, nil) even at EOF).
toRead = 1
}
if int64(len(p)) > toRead {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use a uint64 here for toRead and remaining to avoid an overflow?

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 kept it as a int64 because this variable is also shared with the RequestSizeLimiter, used in router.go as middleware, and takes an int64.

	engine.Use(
		limits.RequestSizeLimiter(config.DefaultHTTPLimit()), // <--- int64 also used here
		loggerFunc(),
		gin.Recovery(),
		cors,
		secureMiddleware(config),
	)

https://github.com/gin-contrib/size/blob/master/size.go#L79

func RequestSizeLimiter(limit int64) gin.HandlerFunc {

Wanted to avoid punting the overflow to when I cast a uint64 to int64. I'm indifferent though, so open to suggestions.

func TestHttpAdapters_NotAUrlError(t *testing.T) {
t.Parallel()

store := leanStore()
Copy link
Contributor

Choose a reason for hiding this comment

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

giphy

if err == io.EOF {
mbr.sawEOF = true
}
if mbr.remaining == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we cal mbr.rc.Read if mbr.remaining == 0?

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 believe this is captured in this comment, but let's chat offline about it:

		// The underlying io.Reader may not return (0, io.EOF)
		// at EOF if the requested size is 0, so read 1 byte
		// instead. The io.Reader docs are a bit ambiguous
		// about the return value of Read when 0 bytes are
		// requested, and {bytes,strings}.Reader gets it wrong
		// too (it returns (0, nil) even at EOF).

Referenced from: https://github.com/gin-contrib/size/blob/master/size.go#L33

@dimroc dimroc merged commit bdc2c22 into master May 29, 2019
@dimroc dimroc deleted the chores/limit-http-get branch May 29, 2019 18:23
mateusz-sekara pushed a commit that referenced this pull request Aug 20, 2024
## Motivation
Static price removal job rollout will be delayed to after 1.5 release.
To unblock db load concerns in 1.4.21 which writes prices to db, we want
to reduce number of token-price related insertions in db.

## Solution
Separate gas price and token price insertion frequency, insert every 10
minutes for token price. 10-min resolution for token price is accurate
enough for our use case.
github-merge-queue bot pushed a commit that referenced this pull request Aug 20, 2024
* Separate token price reporting schedule (#1278)

## Motivation
Static price removal job rollout will be delayed to after 1.5 release.
To unblock db load concerns in 1.4.21 which writes prices to db, we want
to reduce number of token-price related insertions in db.

## Solution
Separate gas price and token price insertion frequency, insert every 10
minutes for token price. 10-min resolution for token price is accurate
enough for our use case.

* Changeset

---------

Co-authored-by: Chunkai Yang <[email protected]>
asoliman92 pushed a commit that referenced this pull request Aug 28, 2024
## Motivation
Static price removal job rollout will be delayed to after 1.5 release.
To unblock db load concerns in 1.4.21 which writes prices to db, we want
to reduce number of token-price related insertions in db.

## Solution
Separate gas price and token price insertion frequency, insert every 10
minutes for token price. 10-min resolution for token price is accurate
enough for our use case.
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.

2 participants