-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
gateway: fix seeker can't seek on specific files #4320
Conversation
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k my understanding of @whyrusleeping's #4286 (comment) is that aside from "that one case", the way we are using the HTTP server has non-trivial performance implications ( iiuc responses essentially can't be streamed until the entire DAG is enumerated ). If the above is correct I much rather wait for the proper fix, than having a workaround go into go-ipfs, precluding time being allocated to fix the real problem :/ |
That is correct, the simplest solution here would be to just trust I'll commit that version of the fix soon |
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
So this PR now does few things:
With these changes large files start downloading immediately after first block is found and are streamed without randomly hanging on blocks that would be downloaded last. |
5484308
to
f6ac552
Compare
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
f6ac552
to
913f964
Compare
core/corehttp/gateway_handler.go
Outdated
return s.sizeReadSeeker.Seek(offset, whence) | ||
} | ||
|
||
func (i *gatewayHandler) serverFile(w http.ResponseWriter, req *http.Request, name string, modtime time.Time, content io.ReadSeeker) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/serverFile/serveFile/
core/corehttp/gateway_handler.go
Outdated
|
||
// If http.ServeContent can't figure out content size it won't write it to the | ||
// responseWriter, Content-Length not being set is a good indicator of this | ||
if req.Method != "HEAD" && w.Header().Get("Content-Length") == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels hacky. I think in this case, ServeContent will have already written an error status and message to the responsewriter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was here before sizeReadSeeker, isn't useful now (maybe as a fallback.. removing for now)
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@mib-kd743naq want to confirm that this works for you? |
@whyrusleeping I won't be able to build a local web gateway for about couple days. But if I do my entire test will consist of firing up a gateway, and making sure I can open each file in https://ipfs.io/ipfs/QmZL4wivfYBEUutxksKgbpPfFkj1tGNMESYLFF2MWcST8Y/Last entry in this subdir errors on latest go-ipfs So if one of you can validate this faster and it works: I am happy ;) |
@mib-kd743naq alright, i'll give it a try! |
Appears to work over here (tested via curl) |
gateway: fix seeker can't seek on specific files
gateway: fix seeker can't seek on specific files This commit was moved from ipfs/kubo@005d243
Fixes #4286
This is the somewhat hacky approach, but it is still way better (IMO) than rewriting
http.ServeContent
just to support that one case.