Mattermost-server: [Help Wanted] [MM-10188] Improve file upload API to stream files instead of loading them entirely into memory

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide, specifically for machine setup and for developer workflow.

---

Notes: Jira ticket

The current version of the file upload code requires loading an entire uploaded file into memory to examine it and then copying it a few times to generate thumbnail images which all takes up a large amount of memory.

This could probably be improved to reduce the memory load caused by uploading large files.

When working on the ticket, remember that we also need to support streaming to S3 via Minio.

Ping @crspeller (@christopher on https://pre-release.mattermost.com) as required to discuss.

[josephGuo]

i'd like improve this .
utils/file_backend.go

type FileBackend interface {
    TestConnection() *model.AppError

    ReadFile(path string) ([]byte, *model.AppError)
    CopyFile(oldPath, newPath string) *model.AppError
    MoveFile(oldPath, newPath string) *model.AppError
    WriteFile(fr io.Reader, path string) *model.AppError
    RemoveFile(path string) *model.AppError

    ListDirectory(path string) (*[]string, *model.AppError)
    RemoveDirectory(path string) *model.AppError
}

utils/file_backend_local.go

func (b *LocalFileBackend) WriteFile(fr io.Reader, path string) *model.AppError {
    return writeFileLocally(fr, filepath.Join(b.directory, path))
}

func writeFileLocally(fr io.Reader, path string) *model.AppError {
    if err := os.MkdirAll(filepath.Dir(path), 0774); err != nil {
        directory, _ := filepath.Abs(filepath.Dir(path))
        return model.NewAppError("WriteFile", "api.file.write_file_locally.create_dir.app_error", nil, "directory="+directory+", err="+err.Error(), http.StatusInternalServerError)
    }
    if fw, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644); err != nil {
        return model.NewAppError("WriteFile", "api.file.write_file_locally.writing.app_error", nil, err.Error(), http.StatusInternalServerError)
    }
    if _, err := io.Copy(fw, fr); err != nil {
        return model.NewAppError("WriteFile", "api.file.write_file_locally.writing.app_error", nil, err.Error(), http.StatusInternalServerError)
    }
    return nil
}

file_backend_s3.go

func (b *S3FileBackend) WriteFile(fr io.Reader, path string) *model.AppError {
    s3Clnt, err := b.s3New()
    if err != nil {
        return model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError)
    }

    var contentType string
    if ext := filepath.Ext(path); model.IsFileExtImage(ext) {
        contentType = model.GetImageMimeType(ext)
    } else {
        contentType = "binary/octet-stream"
    }

    options := s3PutOptions(b.encrypt, contentType)

    if _, err = s3Clnt.PutObject(b.bucket, path, fr, -1, options); err != nil {
        return model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError)
    }

    return nil
}

model/file_info.go

func GetInfoForBytes(name string, fr io.Reader) (*FileInfo, *AppError) {
....
image.DecodeConfig(fr)
...
}

app/file.go

func (a *App) DoUploadFile(now time.Time, rawTeamId string, rawChannelId string, rawUserId string, rawFilename string, fr io.Reader) (*model.FileInfo, *model.AppError) {
    filename := filepath.Base(rawFilename)
    teamId := filepath.Base(rawTeamId)
    channelId := filepath.Base(rawChannelId)
    userId := filepath.Base(rawUserId)

    info, err := model.GetInfoForBytes(filename, fr)
        ...........
    if err := a.WriteFile(fr, info.Path); err != nil {
        return nil, err
    }
}

func (a *App) HandleImages(previewPathList []string, thumbnailPathList []string, fileData []io.Reader) {
.....
}

[crspeller]

@josephGuo If your proposing a fix, please submit as a PR as unformatted code snippets are harder to understand. Thanks!

[sindhugudivada]

Is this still available for others to pick up and work on?

[jasonblais]

Thanks @sindhugudivada for your interest!

@lieut-data I know a PR was opened for this earlier but it was closed (https://github.com/mattermost/mattermost-server/pull/8679). Then, work was prioritized for https://github.com/mattermost/mattermost-server/issues/8838. Is this help wanted issue still valid?

[lieut-data]

@jasonblais, this has been partially addressed by @levb's work in this area, and he may have greater insight into what work remains and if this specific ticket is still available. @levb, can you comment as such?

[levb]

@jasonblais @lieut-data I actually started to prototype this myself over the holidays as I got blocked on other work. (Didn't grab the GH issue because I was just prototyping).

@sindhugudivada - I just posted what I've done so far as a WIP PR ^^. Please feel free to either take it from there or propose alternatives. There is more context in the JIRA ticket https://mattermost.atlassian.net/browse/MM-10188

if you want to take it from here and grab my WIP commits from the above branch or propose alternatives, please feel free. There are more details in the JIRA

[sindhugudivada]

Let me look at it.

Regards
Sindhu

On Wed, Jan 2, 2019 at 7:00 AM Lev [e-mail] wrote:

@jasimmons https://github.com/jasimmons @lieut-data
https://github.com/lieut-data I actually started to prototype this
myself over the holidays as I got blocked on other work. (Didn't grab the
GH issue because I was just prototyping).

@sindhugudivada https://github.com/sindhugudivada - I just posted what
I've done so far as a WIP PR ^^. Please feel free to either take it from
there or propose alternatives. There is more context in the JIRA ticket
https://mattermost.atlassian.net/browse/MM-10188

if you want to take it from here and grab my WIP commits from the above
branch or propose alternatives, please feel free. There are more details in
the

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mattermost/mattermost-server/issues/8641#issuecomment-450885107,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AY-cM2_7qIHDA67Du1daqIFUGwD7WEnMks5u_Ml6gaJpZM4TYmkm
.

[levb]

@sindhugudivada Thank you for taking on this hairy issue. I'd be very happy to help, feel free to ping me on https://community.mattermost.com/

[levb]

@sindhugudivada I updated the PR, it still needs to be fully put together with final configuration details, and a few tests with plugins. Are you still interested in taking this on?

[BK1603]

Is someone working on this? Can i help with this?

[jasonblais]

Thanks @BK1603 for the offer! @sindhugudivada are you currently working on this improvement?

[hanzei]

I think @levb is working on this.

[crspeller]

Yes PR is here: https://github.com/mattermost/mattermost-server/pull/10056

[jasonblais]

Thanks all! You're right, I forgot about the PR. I'll close this issue then, since it's in progress already.

@BK1603 Would you be interested taking another Go ticket? There are a few up for grabs here: https://github.com/mattermost/mattermost-server/issues?utf8=%E2%9C%93&q=is%3Aopen+label%3AGo+label%3A%22Up+For+Grabs%22

[BK1603]

@jasonblais Sure :smile: