Skip to content

feat: try to get content length with zero copy - #81

Open
murasakiakari wants to merge 2 commits into
studio-b12:masterfrom
murasakiakari:fix/unexpected_memory_use_when_getting_content_length
Open

murasakiakari wants to merge 2 commits into
studio-b12:masterfrom
murasakiakari:fix/unexpected_memory_use_when_getting_content_length

Conversation

@murasakiakari

Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Ben Tam <master@murasakiakari.moe>
@murasakiakari

Copy link
Copy Markdown
Contributor Author

@chripo
here is the fix for the previous issue
Thank you for your reviewing

Comment thread utils.go Outdated
case *strings.Reader:
contentLength = int64(reader.Len())
case io.Seeker:
pos, err := reader.Seek(0, io.SeekEnd)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is probably slightly incorrect as the reader could be at a position other than zero:

I think you need to:

  1. startPos, err = reader.Seek(0, io.SeekCurrent) to determine the current position
  2. totalLength, err = reader.Seek(0, io.SeekEnd) to determine the length entire stream
  3. reader.Seek(0, startPos) to restore the original starting position.
  4. return totalLength - startPos - this is how many bytes are remaining

Comment thread utils.go Outdated
}

func GetContentLength(reader io.Reader) (int64, error) {
contentLength := int64(0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

zero is a valid length for some transfers, this should probably return -1 to indicate the length is unknown or return additional bool

Signed-off-by: Ben Tam <master@murasakiakari.moe>
@murasakiakari

murasakiakari commented Dec 19, 2024 •

Copy link
Copy Markdown
Contributor Author

@jkowalski
Thank you for your idea, I have made the changes on getContentLength logic, feel free to take a review

@murasakiakari

Copy link
Copy Markdown
Contributor Author

The branch for bytes.Reader and strings.Reader are removed because they have implemented io.Seeker

@chripo

chripo commented Dec 19, 2024

Copy link
Copy Markdown
Member

thanks for the collaboration!
lgtm.
could you please add some tests?

@jkowalski

Copy link
Copy Markdown
Collaborator

I think you still need to modify c.put to ignore -1 content lengths (and not set the ContentLength field at all). You may also have to have special-case for zero-length buffer.

For the testing I would recommend trying the following case:

  • reader is bytes.Buffer
  • reader is bytes.NewReader()
  • another reader does not support io.Seeker
  • custom request that does support io.Seeker

For each implementation try different lengths:

Try the length of zero and make sure it is explicitly passed as Content-Length: 0 when length Is known and not set at all if length is unknown

Try some non-zero determinate and non-determinate lengths and make sure Content-Length is set accordingly.

bonus points if you can also try a very large stream of 1GB without a length to make sure it's never loaded into memory and streamed all the way to the server.

@murasakiakari

Copy link
Copy Markdown
Contributor Author

Thank for your idea, I will implement it in these days

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.

3 participants