MultipartForm transport should not be seekable by default
- Ngôn ngữ chính
- Go
- Star
- 10.8k
- Fork
- 1.3k
- Merge trung bình
- 2 ngày 36 phút
- Pull request đã merge (30 ngày)
- 26
Mô tả
### Description
Currently, when using file upload, `graphql.Upload` includes the data in a property that is an `io.ReadSeeker`:
https://github.com/99designs/gqlgen/blob/779d7cdd4991e3cf4bf1ecbdea1f02664a56ac8d/graphql/upload.go#L9
Because of that, the `MultipartForm` transport has no choice but to fully process the data from the user:
https://github.com/99designs/gqlgen/blob/779d7cdd4991e3cf4bf1ecbdea1f02664a56ac8d/graphql/handler/transport/http_form.go#L125
Developers here have a choice, controlled by the MaxMemory property: data could either be read fully in memory, or could be stored in a temporary file.
While there are some apps that can benefit by having a seekable stream returned, this is causing a tax on every other app that do _not_ have such a requirement. In fact, many (most?) apps would be totally fine with a "regular" readable stream, that can only be read once (`io.Reader`), and forcing them to accept a seekable stream is ...not nice, adding non-insignificant costs. Storing everything in memory is expensive, of course; while caching on disk is cheaper (not free) but much slower. In both cases, also, consumers need to wait for MultipartForm to complete reading all data before they can start processing it.
Take, for example, an application that allows users to upload large files, say 100MB, 1GB, or more. With the current implementation the app must keep the entire data in memory or on disk (in both cases using quite a bit of not-free resources), then once the client has completed the upload (which depends on their Internet speed), the data can start being processed.
A solution that returned to the consumer a stream, instead, would allow consumers to start processing the data right away (e.g. upload to object storage at the same time as the user is sending the data), and would be "free" on resources.
When MultipartForm exposes only a readable (non-seekable) streams, apps that do not need the ability to go back on the stream can immediately benefit of much improved performance and "zero" costs. Apps that do need seekable streams should still be able to implement that on their own side.
What do you think?
I am happy to propose a PR (I've already toyed around with that). The downside is that it would be backwards-incompatible, and would require users that rely on seekable streams to implement that on their own code instead.
### versions
- `go run github.com/99designs/gqlgen version` 0.17.13
- `go version` 1.18.4
Hướng dẫn đóng góp
Đánh giá
Issue này chưa được đánh giá.