commit 3777f5c68448992a6ed8230f40713d3b31da0413
parent c1585d5f8a57256a330aae4a322468aaccf9d3fa
Author: tobi <31960611+tsmethurst@users.noreply.github.com>
Date: Mon, 19 Sep 2022 13:43:22 +0200
[bugfix] Server and closer bugfixes (#839)
* defer streaming from storage more forcefully
* shut down Server more gracefully
* use command context as server BaseContext
Diffstat:
4 files changed, 47 insertions(+), 29 deletions(-)
diff --git 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
}
- 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)
+ // 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
+ }
+
// 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
@@ -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
@@ -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
@@ -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.
