From 3777f5c68448992a6ed8230f40713d3b31da0413 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Mon, 19 Sep 2022 13:43:22 +0200 Subject: [PATCH 1/3] [bugfix] Server and closer bugfixes (#839) * defer streaming from storage more forcefully * shut down Server more gracefully * use command context as server BaseContext --- internal/api/client/fileserver/servefile.go | 20 +++++++------- internal/media/processingemoji.go | 12 ++++----- internal/media/processingmedia.go | 14 +++++----- internal/router/router.go | 30 ++++++++++++++++----- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/internal/api/client/fileserver/servefile.go b/internal/api/client/fileserver/servefile.go index 0858af3ce..236a2d8ac 100644 --- a/internal/api/client/fileserver/servefile.go +++ b/internal/api/client/fileserver/servefile.go @@ -85,20 +85,22 @@ func (m *FileServer) ServeFile(c *gin.Context) { return } + defer func() { + // if the content is a ReadCloser (ie., it's streamed from storage), close it when we're done + if content.Content != nil { + if closer, ok := content.Content.(io.ReadCloser); ok { + if err := closer.Close(); err != nil { + log.Errorf("ServeFile: error closing readcloser: %s", err) + } + } + } + }() + if content.URL != nil { c.Redirect(http.StatusFound, content.URL.String()) return } - defer func() { - // if the content is a ReadCloser, close it when we're done - if closer, ok := content.Content.(io.ReadCloser); ok { - if err := closer.Close(); err != nil { - log.Errorf("ServeFile: error closing readcloser: %s", err) - } - } - }() - // TODO: if the requester only accepts text/html we should try to serve them *something*. // This is mostly needed because when sharing a link to a gts-hosted file on something like mastodon, the masto servers will // attempt to look up the content to provide a preview of the link, and they ask for text/html. diff --git a/internal/media/processingemoji.go b/internal/media/processingemoji.go index 121f54ddc..e3ed5ce0a 100644 --- a/internal/media/processingemoji.go +++ b/internal/media/processingemoji.go @@ -121,6 +121,12 @@ func (p *ProcessingEmoji) loadStatic(ctx context.Context) error { return p.err } + defer func() { + if err := stored.Close(); err != nil { + log.Errorf("loadStatic: error closing stored full size: %s", err) + } + }() + // we haven't processed a static version of this emoji yet so do it now static, err := deriveStaticEmoji(stored, p.emoji.ImageContentType) if err != nil { @@ -129,12 +135,6 @@ func (p *ProcessingEmoji) loadStatic(ctx context.Context) error { return p.err } - if err := stored.Close(); err != nil { - p.err = fmt.Errorf("loadStatic: error closing stored full size: %s", err) - atomic.StoreInt32(&p.staticState, int32(errored)) - return p.err - } - // put the static in storage if err := p.storage.Put(ctx, p.emoji.ImageStaticPath, static.small); err != nil { p.err = fmt.Errorf("loadStatic: error storing static: %s", err) diff --git a/internal/media/processingmedia.go b/internal/media/processingmedia.go index 26fbc0cea..3f3a68f3f 100644 --- a/internal/media/processingmedia.go +++ b/internal/media/processingmedia.go @@ -145,9 +145,7 @@ func (p *ProcessingMedia) loadThumb(ctx context.Context) error { return p.err } - // whatever happens, close the stream when we're done defer func() { - log.Tracef("loadThumb: closing stored stream %s", p.attachment.URL) if err := stored.Close(); err != nil { log.Errorf("loadThumb: error closing stored full size: %s", err) } @@ -210,6 +208,12 @@ func (p *ProcessingMedia) loadFullSize(ctx context.Context) error { return p.err } + defer func() { + if err := stored.Close(); err != nil { + log.Errorf("loadFullSize: error closing stored full size: %s", err) + } + }() + // decode the image ct := p.attachment.File.ContentType switch ct { @@ -227,12 +231,6 @@ func (p *ProcessingMedia) loadFullSize(ctx context.Context) error { return p.err } - if err := stored.Close(); err != nil { - p.err = fmt.Errorf("loadFullSize: error closing stored full size: %s", err) - atomic.StoreInt32(&p.fullSizeState, int32(errored)) - return p.err - } - // set appropriate fields on the attachment based on the image we derived p.attachment.FileMeta.Original = gtsmodel.Original{ Width: decoded.width, diff --git a/internal/router/router.go b/internal/router/router.go index da00b685d..d66e5f9cd 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -21,6 +21,7 @@ package router import ( "context" "fmt" + "net" "net/http" "time" @@ -37,6 +38,7 @@ const ( writeTimeout = 30 * time.Second idleTimeout = 30 * time.Second readHeaderTimeout = 30 * time.Second + shutdownTimeout = 30 * time.Second ) // Router provides the REST interface for gotosocial, using gin. @@ -128,7 +130,16 @@ func (r *router) Start() { // Stop shuts down the router nicely func (r *router) Stop(ctx context.Context) error { - return r.srv.Shutdown(ctx) + log.Infof("shutting down http router with %s grace period", shutdownTimeout) + timeout, cancel := context.WithTimeout(ctx, shutdownTimeout) + defer cancel() + + if err := r.srv.Shutdown(timeout); err != nil { + return fmt.Errorf("error shutting down http router: %s", err) + } + + log.Info("http router closed connections and shut down gracefully") + return nil } // New returns a new Router with the specified configuration. @@ -174,17 +185,24 @@ func New(ctx context.Context, db db.DB) (Router, error) { return nil, err } - // create the http server here, passing the gin engine as handler + // use the passed-in command context as the base context for the server, + // since we'll never want the server to live past the command anyway + baseCtx := func(_ net.Listener) context.Context { + return ctx + } + bindAddress := config.GetBindAddress() port := config.GetPort() - listen := fmt.Sprintf("%s:%d", bindAddress, port) + addr := fmt.Sprintf("%s:%d", bindAddress, port) + s := &http.Server{ - Addr: listen, - Handler: engine, + Addr: addr, + Handler: engine, // use gin engine as handler ReadTimeout: readTimeout, + ReadHeaderTimeout: readHeaderTimeout, WriteTimeout: writeTimeout, IdleTimeout: idleTimeout, - ReadHeaderTimeout: readHeaderTimeout, + BaseContext: baseCtx, } // We need to spawn the underlying server slightly differently depending on whether lets encrypt is enabled or not. From de26924a4a47fa6b48971cf51f51a5b732745e59 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Mon, 19 Sep 2022 13:59:11 +0200 Subject: [PATCH 2/3] don't error out if storage key already exists (#840) --- internal/media/processingemoji.go | 4 ++-- internal/media/processingmedia.go | 4 ++-- internal/storage/local.go | 13 +++++++++++-- internal/storage/storage.go | 1 + 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/internal/media/processingemoji.go b/internal/media/processingemoji.go index e3ed5ce0a..7ffb6d7fa 100644 --- a/internal/media/processingemoji.go +++ b/internal/media/processingemoji.go @@ -136,7 +136,7 @@ func (p *ProcessingEmoji) loadStatic(ctx context.Context) error { } // put the static in storage - if err := p.storage.Put(ctx, p.emoji.ImageStaticPath, static.small); err != nil { + if err := p.storage.Put(ctx, p.emoji.ImageStaticPath, static.small); err != nil && err != storage.ErrAlreadyExists { p.err = fmt.Errorf("loadStatic: error storing static: %s", err) atomic.StoreInt32(&p.staticState, int32(errored)) return p.err @@ -217,7 +217,7 @@ func (p *ProcessingEmoji) store(ctx context.Context) error { multiReader := io.MultiReader(bytes.NewBuffer(firstBytes), reader) // store this for now -- other processes can pull it out of storage as they please - if err := p.storage.PutStream(ctx, p.emoji.ImagePath, multiReader); err != nil { + if err := p.storage.PutStream(ctx, p.emoji.ImagePath, multiReader); err != nil && err != storage.ErrAlreadyExists { return fmt.Errorf("store: error storing stream: %s", err) } diff --git a/internal/media/processingmedia.go b/internal/media/processingmedia.go index 3f3a68f3f..e537c8301 100644 --- a/internal/media/processingmedia.go +++ b/internal/media/processingmedia.go @@ -162,7 +162,7 @@ func (p *ProcessingMedia) loadThumb(ctx context.Context) error { // put the thumbnail in storage log.Tracef("loadThumb: storing new thumbnail %s", p.attachment.URL) - if err := p.storage.Put(ctx, p.attachment.Thumbnail.Path, thumb.small); err != nil { + if err := p.storage.Put(ctx, p.attachment.Thumbnail.Path, thumb.small); err != nil && err != storage.ErrAlreadyExists { p.err = fmt.Errorf("loadThumb: error storing thumbnail: %s", err) atomic.StoreInt32(&p.thumbState, int32(errored)) return p.err @@ -341,7 +341,7 @@ func (p *ProcessingMedia) store(ctx context.Context) error { p.attachment.File.FileSize = fileSize // store this for now -- other processes can pull it out of storage as they please - if err := p.storage.PutStream(ctx, p.attachment.File.Path, clean); err != nil { + if err := p.storage.PutStream(ctx, p.attachment.File.Path, clean); err != nil && err != storage.ErrAlreadyExists { return fmt.Errorf("store: error storing stream: %s", err) } diff --git a/internal/storage/local.go b/internal/storage/local.go index 8ed3ca8d8..103233219 100644 --- a/internal/storage/local.go +++ b/internal/storage/local.go @@ -24,6 +24,7 @@ import ( "net/url" "codeberg.org/gruf/go-store/kv" + "codeberg.org/gruf/go-store/storage" ) type Local struct { @@ -39,11 +40,19 @@ func (l *Local) GetStream(ctx context.Context, key string) (io.ReadCloser, error } func (l *Local) PutStream(ctx context.Context, key string, r io.Reader) error { - return l.KVStore.PutStream(key, r) + err := l.KVStore.PutStream(key, r) + if err == storage.ErrAlreadyExists { + return ErrAlreadyExists + } + return err } func (l *Local) Put(ctx context.Context, key string, value []byte) error { - return l.KVStore.Put(key, value) + err := l.KVStore.Put(key, value) + if err == storage.ErrAlreadyExists { + return ErrAlreadyExists + } + return err } func (l *Local) Delete(ctx context.Context, key string) error { diff --git a/internal/storage/storage.go b/internal/storage/storage.go index 7a4ebc6c3..7bd4c18aa 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -34,6 +34,7 @@ import ( ) var ErrNotSupported = errors.New("driver does not suppport functionality") +var ErrAlreadyExists = errors.New("storage key already exists") // Driver implements the functionality to store and retrieve blobs // (images,video,audio) From 8c20626c51b1d986b351e9f2e188675b54245420 Mon Sep 17 00:00:00 2001 From: Phil Hagelberg Date: Tue, 20 Sep 2022 01:52:02 -0700 Subject: [PATCH 3/3] [docs] Add --config-path to example CLI commands where needed. (#843) Previously we had a few examples referring to --config-file (which is not accepted) but most were missing it altogether. Put this argument last in all the examples. Also replaced "./example.json" with just "example.json" in the import/export examples because the "./" was unnecessary. --- docs/admin/cli.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/admin/cli.md b/docs/admin/cli.md index 7a71f2fcd..e708a8a29 100644 --- a/docs/admin/cli.md +++ b/docs/admin/cli.md @@ -62,7 +62,8 @@ Example: gotosocial admin account create \ --username some_username \ --email someuser@example.org \ - --password 'somelongandcomplicatedpassword' + --password 'somelongandcomplicatedpassword' \ + --config-path config.yaml ``` ### gotosocial admin account confirm @@ -85,7 +86,7 @@ Flags: Example: ```bash -gotosocial admin account confirm --username some_username +gotosocial admin account confirm --username some_username --config-path config.yaml ``` ### gotosocial admin account promote @@ -108,7 +109,7 @@ Flags: Example: ```bash -gotosocial admin account promote --username some_username +gotosocial admin account promote --username some_username --config-path config.yaml ``` ### gotosocial admin account demote @@ -131,7 +132,7 @@ Flags: Example: ```bash -gotosocial admin account demote --username some_username +gotosocial admin account demote --username some_username --config-path config.yaml ``` ### gotosocial admin account disable @@ -154,7 +155,7 @@ Flags: Example: ```bash -gotosocial admin account disable --username some_username +gotosocial admin account disable --username some_username --config-path config.yaml ``` ### gotosocial admin account suspend @@ -179,7 +180,7 @@ Flags: Example: ```bash -gotosocial admin account suspend --username some_username +gotosocial admin account suspend --username some_username --config-path config.yaml ``` ### gotosocial admin account password @@ -203,7 +204,7 @@ Flags: Example: ```bash -gotosocial admin account password --username some_username --pasword some_really_good_password +gotosocial admin account password --username some_username --pasword some_really_good_password --config-path config.yaml ``` ### gotosocial admin export @@ -228,7 +229,7 @@ Flags: Example: ```bash -gotosocial admin export --config-file ./config.yaml --path ./example.json +gotosocial admin export --path example.json --config-path config.yaml ``` `example.json`: @@ -276,5 +277,5 @@ Flags: Example: ```bash -gotosocial admin import --config-file ./config.yaml --path ./example.json +gotosocial admin import --path example.json --config-path config.yaml ```