Stream object downloads instead of buffering objects in memory by dnerdy · Pull Request #829 · fsouza/fake-gcs-server

dnerdy · 2022-06-15T15:19:07Z

Fixes #828.

This PR makes it so objects in the FS backend are streamed from disk
instead of being loaded into memory. This drastically reduces memory
usage when working with large files, especially when performing small
concurrent range reads on a large file.

A StreamingObject type has been added to both the fakestorage and
backend packages. This type contains a io.ReadSeekCloser that can be
used to access object data.

This change sets the stage for fixing #669 and #397. #669 isn't fixed
because it will require re-working of how resumable uploads are stored
(though other upload types have been made streaming in this PR). Also,
fixing #397 would require changing how initial objects are specified
when starting the server, which with this approach would require
changing the public API.

Regarding the public API: the server methods for working with objects
have been changed to use StreamingObject instead of Object. This is a
breaking change. It would be possible to make streaming backward
compatible by adding some metadata and methods to Object (to distinguish
between streaming and buffered objects), but I went with this approach
because it made identifying the code that needed to be updated easier;
the compiler pointed out the locations to update instead of needing to
track down runtime and test errors. An additional improvement, which is
compatible with the backward compatible approach, would be to add an
OpenForReading method on Object so callers that are currently
responsible for closing objects also explicitly open objects (which
would make the responsibility clearer). Anyway, I wanted to get this
initial approach up for feedback before putting in additional work.

Fixes fsouza#828. This PR makes it so objects in the FS backed are streamed from disk instead of being loaded into memory. This drastically reduces memory usage when working with large files, especially when performing small concurrent range reads on a large file. A StreamingObject type has been added to both the fakestorage and API packages. This type contains a io.ReadSeekCloser that can be used to access object data. This change sets the stage for fixing fsouza#669 and fsouza#397. fsouza#669 isn't fixed because it will require re-working of how resumable uploads are stored (though other upload types have been made streaming in this PR). Also, fixing fsouza#397 would require changing how initial objects are specified when starting the server, which with this approach would require changing the public API. Regarding the public API: the server methods for working with objects have been changed to use StreamingObject instead of Object. This is a breaking change. It would be possible to make streaming backward compatible by adding some metadata and methods to Object (to distinguish between streaming and buffered objects), but I went with this approach because it made identifying the code that needed to be updated easier; the compiler pointed out the locations to update instead of needing to track down runtime and test errors. An additional improvement, which is compatible with the backward compatible approach, would be to add an OpenForReading method on Object so callers that are currently responsible for closing objects also explicitly open objects (which would make the responsibility clearer). Anyway, I wanted to get this initial approach up for feedback before putting in additional work.

StevenACoffman · 2022-06-20T17:48:42Z

@fsouza what do you think of this? It has made a very large performance improvement in our internal testing.

dnerdy · 2022-06-21T16:03:13Z

Our engineers have been using this change in our dev environment for about a week (probably millions of requests). Folks use Linux and macOS. It would be helpful if someone in the community could also test the change on Windows. Windows has very different file open, rename and delete semantics. I've used github.com/alexbrainman/goissue34681 (found via golang/go#34681), though I'm not sure if this works well with multiple readers.

The strategy taken in this code is as follows:

Obtain lock when accessing an object
Open backing file
Unlock
If later the object is updated or deleted, the backing file is replaced or deleted (under a different lock)
Open readers (which no longer hold a lock) continue to read from the original open backing file

This allows a slow reader (or multiple readers) to download the original file even if the backing file has been modified or deleted.

fsouza · 2022-06-24T21:43:59Z

@fsouza what do you think of this? It has made a very large performance improvement in our internal testing.

On a quick glance, this looks sane. I'm a bit slow to respond right now, but I'll try to come back to it later today or some time tomorrow.

I also don't have access to any Windows hosts, so if we don't get any volunteers the best strategy will probably be to merge the PR and wait for bug reports 🙈

fsouza

Looks good overall, just want to make sure we don't ship a breaking change in the lib API.

fsouza · 2022-06-26T21:42:24Z

 		Size            int64             `json:"size,string"`
 		ContentType     string            `json:"contentType"`
 		ContentEncoding string            `json:"contentEncoding"`
-		Content         []byte            `json:"-"`


lol didn't even realize we did this

dnerdy · 2022-08-09T21:54:54Z

@fsouza are you still interested in merging this PR? I'm curious what your opinion is about other breaking changes in this PR. You mentioned CreateObject, and @StevenACoffman changed the name back in #882, though the function signature is still different: it takes a StreamingObject. Some of the other functions also have different signatures. Are you okay with any breaking changes, or do you want the exported functions to stay backward compatible (which would mean creating new streaming variants)?

fsouza · 2022-08-11T16:38:21Z

@fsouza are you still interested in merging this PR? I'm curious what your opinion is about other breaking changes in this PR. You mentioned CreateObject, and @StevenACoffman changed the name back in #882, though the function signature is still different: it takes a StreamingObject. Some of the other functions also have different signatures. Are you okay with any breaking changes, or do you want the exported functions to stay backward compatible (which would mean creating new streaming variants)?

Hey, apologies for the silence, I was on a long break, but coming back now! Yeah, I wanted to keep it fully backwards compatible. We could keep the streaming variant private unless yall are already using fake-gcs-server as a library for this. Like, keep it private unless someone actually needs it? Not sure what you think.

dnerdy · 2022-08-12T16:56:02Z

We could keep the streaming variant private unless yall are already using fake-gcs-server as a library for this. Like, keep it private unless someone actually needs it? Not sure what you think.

No worries! And apologies for taking a while to circle back to this myself. Yeah, that seems reasonable. I'll work on reverting the existing APIs and I'll double check our usage to see if anything new needs to be exported (I suspect not).

fsouza · 2022-08-14T23:33:29Z

@dnerdy I'm curious on how you and @StevenACoffman would like to proceed. Should we close #882 and continue working on this PR?

dnerdy · 2022-09-02T02:01:46Z

@fsouza The original buffered APIs have been restored:

CreateObject
GetObject
GetObjectWithGeneration

The new streaming variants are named <X>Streaming, i.e.

CreateObjectStreaming
GetObjectStreaming
GetObjectWithGenerationStreaming

Internally, the server uses streaming objects and the objects are converted to buffered objects in the restored methods.

I've moved the documentation from the buffered variants to the streaming variants, to encourage their use, and updated the documentation for the buffered methods to point to the streaming variants.

One additional change I made to CreateObjectStreaming is that it returns an error instead of panicking when an error occurs.

I've also merged in main and updated the code to use the new metadata interface.

fsouza

This looks good in general, I have some minor comments left. Thank you very much for pushing this through!

fsouza · 2022-09-02T04:16:12Z


-	if err = os.WriteFile(path, obj.Content, 0o600); err != nil {
-		return Object{}, err
+	tempFile, err := os.CreateTemp("", "fake-gcs-object")


If we want to guarantee that rename will work, we'll likely want to specify the rename, otherwise the temporary directory may be in a different mount point. No need to overthink it though, we could address it later if someone raises the issue :)

fsouza · 2022-09-02T04:20:14Z

@@ -0,0 +1,21 @@
+//go:build !windows
+// +build !windows


Hmm now that we only need to support Go 1.18 and 1.19, we can get rid of +build (go fix should do it automatically I think).

fsouza · 2022-09-02T04:21:05Z

+	"path/filepath"
+	"time"
+
+	"github.com/alexbrainman/goissue34681"


This feels wrong lol I wonder if the author will keep that around. I guess that in the worst case it'll be cached in the goproxy and we can fork it.

Applying it myself so we can ship this. I'll run `go fix` after merging. Co-authored-by: Steve Coffman <StevenACoffman@users.noreply.github.com>

fsouza

Thank you very much for contributing!

dnerdy changed the title ~~Stream object downloads instead of buffering object in memory~~ Jun 15, 2022

dnerdy added 6 commits June 15, 2022 09:02

Fix typo in xattr_windows.go

1c2962e

Fix staticcheck lint errors.

f354f3d

Attempt to fix renaming over open files on Windows

07f8045

Add build directive to rename_unix.go

1c9ecbb

Close objects when listing objects and in tests

d0e24d3

Use compatOpenForWritingAllowingRename for temp file

3d78ac9

dnerdy force-pushed the streaming-downloads branch from f650f66 to 3d78ac9 Compare June 16, 2022 03:23

dnerdy added 2 commits June 15, 2022 20:35

Close temp file

227228f

Move the Windows "temporary delete path" to the temp dir

b15c8a5

dnerdy force-pushed the streaming-downloads branch from 3da2a5c to b15c8a5 Compare June 16, 2022 03:49

Close initial objects

4f18d8d

fsouza reviewed Jun 26, 2022

View reviewed changes

StevenACoffman reviewed Jun 27, 2022

View reviewed changes

Comment thread fakestorage/upload.go Outdated

StevenACoffman mentioned this pull request Aug 4, 2022

Streaming downloads #882

Closed

dnerdy added 2 commits September 1, 2022 18:30

Bring back original non-streaming public APIs

6da0e2c

Merge remote-tracking branch 'upstream/main' into streaming-downloads

fd2903c

fsouza reviewed Sep 2, 2022

View reviewed changes

Apply suggestions from code review

ba4e415

Applying it myself so we can ship this. I'll run `go fix` after merging. Co-authored-by: Steve Coffman <StevenACoffman@users.noreply.github.com>

fsouza approved these changes Sep 2, 2022

View reviewed changes

fsouza merged commit 12a03cf into fsouza:main Sep 2, 2022

fsouza mentioned this pull request Sep 5, 2022

FATA[0000] couldn't start the server: invalid cross-device link #912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stream object downloads instead of buffering objects in memory - #829

Stream object downloads instead of buffering objects in memory#829
fsouza merged 13 commits into
fsouza:mainfrom
dnerdy:streaming-downloads

dnerdy commented Jun 15, 2022 •

edited

Loading

StevenACoffman commented Jun 20, 2022

dnerdy commented Jun 21, 2022

fsouza commented Jun 24, 2022 •

edited

Loading

fsouza left a comment

Uh oh!

fsouza Jun 26, 2022

Uh oh!

dnerdy commented Aug 9, 2022

fsouza commented Aug 11, 2022

dnerdy commented Aug 12, 2022

fsouza commented Aug 14, 2022

dnerdy commented Sep 2, 2022

fsouza left a comment

fsouza Sep 2, 2022

Uh oh!

fsouza Sep 2, 2022

fsouza Sep 2, 2022

fsouza left a comment

Labels

3 participants

Conversation

dnerdy commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

StevenACoffman commented Jun 20, 2022

dnerdy commented Jun 21, 2022

fsouza commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fsouza left a comment

Choose a reason for hiding this comment

Uh oh!

fsouza Jun 26, 2022

Choose a reason for hiding this comment

Uh oh!

dnerdy commented Aug 9, 2022

fsouza commented Aug 11, 2022

dnerdy commented Aug 12, 2022

fsouza commented Aug 14, 2022

dnerdy commented Sep 2, 2022

fsouza left a comment

Choose a reason for hiding this comment

fsouza Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

fsouza Sep 2, 2022

Choose a reason for hiding this comment

fsouza Sep 2, 2022

Choose a reason for hiding this comment

fsouza left a comment

Choose a reason for hiding this comment

Labels

3 participants

dnerdy commented Jun 15, 2022 •

edited

Loading

fsouza commented Jun 24, 2022 •

edited

Loading