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

fix(server): Set content-length and use faster fetch API for file transfers #2800

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

nellh
Copy link
Contributor

@nellh nellh commented Apr 12, 2023

This replaces the superagent variant of getFile with the fetch equivalent. Testing locally with a 10GB nifti this is a little faster than the superagent version, which had been used for performance reasons originally.

# Superagent
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  9.8G    0  9.8G    0     0   975M      0 --:--:--  0:00:10 --:--:--  959M
# Fetch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  9.8G    0  9.8G    0     0  1392M      0 --:--:--  0:00:07 --:--:-- 1412M

This change also adds a content-length header to the response, allowing an HTTP client to detect errors when the connection is closed early.

This may fix #2798, I think this is worth trying in staging (it isn't reproducible locally).

@nellh nellh requested a review from effigies April 12, 2023 23:15
@effigies
Copy link
Contributor

No idea if this is sensible or plausible, but would it work to redirect to the S3 URL for snapshots? (Will look at this PR tomorrow.)

@nellh
Copy link
Contributor Author

nellh commented Apr 13, 2023

No idea if this is sensible or plausible, but would it work to redirect to the S3 URL for snapshots? (Will look at this PR tomorrow.)

I think I have a quick improvement for that in mind, I'll make that separately.

Copy link
Contributor

@effigies effigies left a comment

Choose a reason for hiding this comment

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

This reads sensibly. If I understand it correctly, it takes an external URL, translates it to an internal URL and then proxies the internal stream back out to the external request.

Probably want to remove L11-L12 comment.

@nellh nellh merged commit 47d10b4 into master Apr 13, 2023
@nellh nellh deleted the 2798-download-length-fixes branch April 13, 2023 17:09
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.

Web downloads capped at 7.5GB
2 participants