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

feat: support mulit/range #1398

Merged
merged 4 commits into from Oct 30, 2022

Conversation

byene0923
Copy link
Contributor

@byene0923 byene0923 commented Oct 17, 2022

pr for #1312

  • change SetByteRange, support multipart/byteranges
  • change bigFileReader and fsSmallFileReader which only handle complete files
  • add smallRangeReader and bigRangeReader which only handle range files
  • change byteRangeUpdater interface to byteRangeHandler
    test
  • change TestFSByteRangeSingleThread, add bigFileReader and fsSmallFileReader test of read and WriteTo
  • add TestFSMultiByteRangeConcurrent and TestFSMultiByteRangeSingleThread
  • change TestParseByteMultiRangeSuccess and TestParseByteRangeError

header.go Outdated Show resolved Hide resolved
header_test.go Outdated Show resolved Hide resolved
1. lint code
2. add SetByteRanges method
v = &smallRangeReader{
sta: make([]int, 64),
end: make([]int, 64),
buf: make([]byte, 4096),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 4096, why not maxSmallFileSize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4096 keep the same with copyBufPool, this use for copyZeroAlloc

cLen = len(p[cPos:])
n = copy(p[cPos:], r.buf)
cPos += n
if len(r.buf) > cLen {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if cLen is less than the length of the range header? Shouldn't this check exclude the header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a bit complex
the multibyte/range like

--3d6b6a416f9b5
Content-Type: text/html
Content-Range: bytes 0-15/1270

<!DOCTYPE html>
--3d6b6a416f9b5
Content-Type: text/html
Content-Range: bytes 0-10/1270

<!DOCTYPE 
--3d6b6a416f9b5--

for each range, we need to add boundary, Content-Type, Content-Range first, then add http content, at last range, we need to and boundary--

however, the length of p is limited, so we need to check whether p is full when we add multiRangeBodyHeader which contain boundary, Content-Type, Content-Range and http content, and multiRangeBodyEnd which contain boundary--

the r.buf is the rest content of multiRangeBodyHeader or multiRangeBodyEnd that has not copy into p at last time.
cLen is the rest lengh of p, and we have done n = copy(p[cPos:], r.buf), if cLen is less then len(r.buf), we can copy all the content of r.buf to p

by the way,
method read like a streaming which the p is limited
first, we read the multiRangeBodyHeader, then we read http content, if we are in last range, we read the multiRangeBodyEnd

the code which handle range content begins in line fs.go 1603

if rf, ok := w.(io.ReaderFrom); ok {
return rf.ReadFrom(r)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still relevant? And if so why not do it in smallRangeReader as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for bytebufferpool, do as this

bb := bytebufferpool.Get()
_, err := reader.WriteTo(bb)

and smallRangeReader has also do it, in fs.go line 1690. smallRangeReader do the same as smallFileReader, handle directory first, then handle readfrom, last handle write

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

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

It seems like TestFSMultiByteRangeConcurrent always gets a timeout, can you fix that any other tests that might fail?

@byene0923
Copy link
Contributor Author

sure, the test number of concurrent and multirange may be too large

@erikdubbelboer erikdubbelboer merged commit a468a7d into valyala:master Oct 30, 2022
14 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

erikdubbelboer added a commit that referenced this pull request Nov 28, 2022
erikdubbelboer added a commit that referenced this pull request Nov 28, 2022
@erikdubbelboer
Copy link
Collaborator

@byene0923 I had to revert this change as it caused a critical bug: #1445

Can you have a look at adding a test case for this and fixing it?

@byene0923
Copy link
Contributor Author

ok, sorry for missing the full tests. i will look deep into it

@MindMayhem
Copy link

MindMayhem commented Nov 28, 2022

@byene0923 maybe it will be useful to you, but when we request large static file, then in request headers an interesting heading appears - Range: 3896-3896
on others files there's no such header

also, it's worth noting that my file was 5mb in size and returned was 200 bytes

@byene0923
Copy link
Contributor Author

@MindMayhem thanks a lot ,but i can not reproduce it by your files in #1445 now. should any changes i do in your code ?

@MindMayhem
Copy link

@byene0923 that's not my example, but i pretty sure it shows that error, just check your browser console
i hope @smalloff can provide more info to you

@erikdubbelboer
Copy link
Collaborator

I was able to reproduce it on MacOS using the files in the zipfile in this comment: #1445 (comment) Only thing I did was go mod tidy and go run main.go.

@li-jin-gou
Copy link
Contributor

li-jin-gou commented Nov 29, 2022

I was able to reproduce it on MacOS using the files in the zipfile in this comment: #1445 (comment) Only thing I did was go mod tidy and go run main.go.

I'll try.

@byene0923
Copy link
Contributor Author

thanks a lot to @MindMayhem and @erikdubbelboer, i reproduce it, i will look into it soon

@byene0923
Copy link
Contributor Author

byene0923 commented Nov 29, 2022

hello, @erikdubbelboer @MindMayhem i have found the bug in fasthttp@1.42.0 fs.go, line 749

func (r *fsSmallFileReader) WriteTo(w io.Writer) (int64, error) {
	if r.offset != 0 {
		panic("BUG: non-zero offset! Read() mustn't be called before WriteTo()")
	}

when file size in (4096, 8192) will cause this bug. because writeTo() will be called twice, and the second call r.offset is not zero, so this will cause panic, when file size > 8192, it use bigFileReader, when file size < 4096, writeTo() will be called only once, because the buf from copyBufPool and size is 4096

many thanks to @MindMayhem and @smalloff ! i will fix it soon and add unit tests

@byene0923
Copy link
Contributor Author

i delete this check. and i'm doing the unit test, you could change the go.mod of the zipfile in this comment: #1445 (comment)

just add this at the last line of go.mod

replace github.com/valyala/fasthttp => github.com/byene0923/fasthttp v1.23.1-0.20221202163206-7e40fb23488f

if @erikdubbelboer @MindMayhem have interest, can have a try

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.

None yet

4 participants