commit c9d893fec18fbb638eda1ee03776ae34c562f39b
parent 8942a70856acd6944cec54addb35189aa97c8810
Author: tobi <31960611+tsmethurst@users.noreply.github.com>
Date: Wed, 23 Nov 2022 22:40:07 +0100
[feature/performance] Fail fast when doing remote transport calls inside incoming request contexts (#1119)
* [feature/performance] Fail fast when doing remote transport calls inside incoming request contexts
* [chore] Reduce outgoing request timeout to 15s
* log error messages when fastfailing
* use context.Value() instead of wrapped context, wrap error with fastfail instead of extra log entry
* add fast-fail context key test
Signed-off-by: kim <grufwub@gmail.com>
Co-authored-by: kim <grufwub@gmail.com>
Diffstat:
17 files changed, 141 insertions(+), 24 deletions(-)
diff --git a/internal/federation/authenticate.go b/internal/federation/authenticate.go
@@ -216,7 +216,7 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU
}
log.Tracef("proceeding with dereference for uncached public key %s", requestingPublicKeyID)
- trans, err := f.transportController.NewTransportForUsername(ctx, requestedUsername)
+ trans, err := f.transportController.NewTransportForUsername(transport.WithFastfail(ctx), requestedUsername)
if err != nil {
errWithCode := gtserror.NewErrorInternalError(fmt.Errorf("error creating transport for %s: %s", requestedUsername, err))
log.Debug(errWithCode)
diff --git a/internal/federation/dereferencing/account.go b/internal/federation/dereferencing/account.go
@@ -87,6 +87,11 @@ type GetRemoteAccountParams struct {
//
// If a local account is passed into this function for whatever reason (hey, it happens!), then it
// will be returned from the database without making any remote calls.
+//
+// Even if a fastfail context is used, and something goes wrong, an account might still be returned instead
+// of an error, if we already had the account in our database (in other words, if we just needed to try
+// fingering/refreshing the account again). The rationale for this is that it's more useful to be able
+// to provide *something* to the caller, even if that something is not necessarily 100% up to date.
func (d *deref) GetRemoteAccount(ctx context.Context, params GetRemoteAccountParams) (foundAccount *gtsmodel.Account, err error) {
/*
In this function we want to retrieve a gtsmodel representation of a remote account, with its proper
diff --git a/internal/federation/federatingprotocol.go b/internal/federation/federatingprotocol.go
@@ -34,6 +34,7 @@ import (
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
"github.com/superseriousbusiness/gotosocial/internal/log"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
"github.com/superseriousbusiness/gotosocial/internal/uris"
"github.com/superseriousbusiness/gotosocial/internal/util"
)
@@ -191,7 +192,7 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
}
// we don't have an entry for this instance yet so dereference it
- i, err = f.GetRemoteInstance(ctx, username, &url.URL{
+ i, err = f.GetRemoteInstance(transport.WithFastfail(ctx), username, &url.URL{
Scheme: publicKeyOwnerURI.Scheme,
Host: publicKeyOwnerURI.Host,
})
@@ -205,7 +206,7 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
}
}
- requestingAccount, err := f.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ requestingAccount, err := f.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: username,
RemoteAccountID: publicKeyOwnerURI,
})
diff --git a/internal/httpclient/client.go b/internal/httpclient/client.go
@@ -95,7 +95,7 @@ func New(cfg Config) *Client {
var c Client
d := &net.Dialer{
- Timeout: 30 * time.Second,
+ Timeout: 15 * time.Second,
KeepAlive: 30 * time.Second,
Resolver: &net.Resolver{},
}
diff --git a/internal/processing/account/get.go b/internal/processing/account/get.go
@@ -29,6 +29,7 @@ import (
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtserror"
"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
)
func (p *processor) Get(ctx context.Context, requestingAccount *gtsmodel.Account, targetAccountID string) (*apimodel.Account, gtserror.WithCode) {
@@ -93,7 +94,7 @@ func (p *processor) getAccountFor(ctx context.Context, requestingAccount *gtsmod
return nil, gtserror.NewErrorInternalError(fmt.Errorf("error parsing url %s: %s", targetAccount.URI, err))
}
- a, err := p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ a, err := p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: requestingAccount.Username,
RemoteAccountID: targetAccountURI,
RemoteAccountHost: targetAccount.Domain,
diff --git a/internal/processing/federation/getfollowers.go b/internal/processing/federation/getfollowers.go
@@ -26,6 +26,7 @@ import (
"github.com/superseriousbusiness/activity/streams"
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtserror"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
)
func (p *processor) GetFollowers(ctx context.Context, requestedUsername string, requestURL *url.URL) (interface{}, gtserror.WithCode) {
@@ -41,7 +42,7 @@ func (p *processor) GetFollowers(ctx context.Context, requestedUsername string,
return nil, errWithCode
}
- requestingAccount, err := p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ requestingAccount, err := p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: requestedUsername,
RemoteAccountID: requestingAccountURI,
})
diff --git a/internal/processing/federation/getfollowing.go b/internal/processing/federation/getfollowing.go
@@ -26,6 +26,7 @@ import (
"github.com/superseriousbusiness/activity/streams"
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtserror"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
)
func (p *processor) GetFollowing(ctx context.Context, requestedUsername string, requestURL *url.URL) (interface{}, gtserror.WithCode) {
@@ -41,7 +42,7 @@ func (p *processor) GetFollowing(ctx context.Context, requestedUsername string,
return nil, errWithCode
}
- requestingAccount, err := p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ requestingAccount, err := p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: requestedUsername,
RemoteAccountID: requestingAccountURI,
})
diff --git a/internal/processing/federation/getoutbox.go b/internal/processing/federation/getoutbox.go
@@ -27,6 +27,7 @@ import (
"github.com/superseriousbusiness/gotosocial/internal/db"
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtserror"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
)
func (p *processor) GetOutbox(ctx context.Context, requestedUsername string, page bool, maxID string, minID string, requestURL *url.URL) (interface{}, gtserror.WithCode) {
@@ -42,7 +43,7 @@ func (p *processor) GetOutbox(ctx context.Context, requestedUsername string, pag
return nil, errWithCode
}
- requestingAccount, err := p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ requestingAccount, err := p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: requestedUsername,
RemoteAccountID: requestingAccountURI,
})
diff --git a/internal/processing/federation/getstatus.go b/internal/processing/federation/getstatus.go
@@ -26,6 +26,7 @@ import (
"github.com/superseriousbusiness/activity/streams"
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtserror"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
)
func (p *processor) GetStatus(ctx context.Context, requestedUsername string, requestedStatusID string, requestURL *url.URL) (interface{}, gtserror.WithCode) {
@@ -41,7 +42,7 @@ func (p *processor) GetStatus(ctx context.Context, requestedUsername string, req
return nil, errWithCode
}
- requestingAccount, err := p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ requestingAccount, err := p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: requestedUsername,
RemoteAccountID: requestingAccountURI,
})
diff --git a/internal/processing/federation/getstatusreplies.go b/internal/processing/federation/getstatusreplies.go
@@ -28,6 +28,7 @@ import (
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtserror"
"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
)
func (p *processor) GetStatusReplies(ctx context.Context, requestedUsername string, requestedStatusID string, page bool, onlyOtherAccounts bool, minID string, requestURL *url.URL) (interface{}, gtserror.WithCode) {
@@ -43,7 +44,7 @@ func (p *processor) GetStatusReplies(ctx context.Context, requestedUsername stri
return nil, errWithCode
}
- requestingAccount, err := p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ requestingAccount, err := p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: requestedUsername,
RemoteAccountID: requestingAccountURI,
})
diff --git a/internal/processing/federation/getuser.go b/internal/processing/federation/getuser.go
@@ -27,6 +27,7 @@ import (
"github.com/superseriousbusiness/activity/streams/vocab"
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtserror"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
"github.com/superseriousbusiness/gotosocial/internal/uris"
)
@@ -53,7 +54,7 @@ func (p *processor) GetUser(ctx context.Context, requestedUsername string, reque
// if we're not already handshaking/dereferencing a remote account, dereference it now
if !p.federator.Handshaking(ctx, requestedUsername, requestingAccountURI) {
- requestingAccount, err := p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ requestingAccount, err := p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: requestedUsername,
RemoteAccountID: requestingAccountURI,
})
diff --git a/internal/processing/media/getfile.go b/internal/processing/media/getfile.go
@@ -31,6 +31,7 @@ import (
"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
"github.com/superseriousbusiness/gotosocial/internal/log"
"github.com/superseriousbusiness/gotosocial/internal/media"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
"github.com/superseriousbusiness/gotosocial/internal/uris"
)
@@ -141,11 +142,11 @@ func (p *processor) getAttachmentContent(ctx context.Context, requestingAccount
// large version and derive a thumbnail from it, so use the normal recaching procedure: fetch the media,
// process it, then return the thumbnail data
data = func(innerCtx context.Context) (io.ReadCloser, int64, error) {
- transport, err := p.transportController.NewTransportForUsername(innerCtx, requestingUsername)
+ t, err := p.transportController.NewTransportForUsername(innerCtx, requestingUsername)
if err != nil {
return nil, 0, err
}
- return transport.DereferenceMedia(innerCtx, remoteMediaIRI)
+ return t.DereferenceMedia(transport.WithFastfail(innerCtx), remoteMediaIRI)
}
} else {
// if it's the full-sized version being requested, we can cheat a bit by streaming data to the user as
@@ -172,12 +173,12 @@ func (p *processor) getAttachmentContent(ctx context.Context, requestingAccount
attachmentContent.Content = io.NopCloser(bufferedReader)
data = func(innerCtx context.Context) (io.ReadCloser, int64, error) {
- transport, err := p.transportController.NewTransportForUsername(innerCtx, requestingUsername)
+ t, err := p.transportController.NewTransportForUsername(innerCtx, requestingUsername)
if err != nil {
return nil, 0, err
}
- readCloser, fileSize, err := transport.DereferenceMedia(innerCtx, remoteMediaIRI)
+ readCloser, fileSize, err := t.DereferenceMedia(transport.WithFastfail(innerCtx), remoteMediaIRI)
if err != nil {
return nil, 0, err
}
diff --git a/internal/processing/search.go b/internal/processing/search.go
@@ -34,6 +34,7 @@ import (
"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
"github.com/superseriousbusiness/gotosocial/internal/log"
"github.com/superseriousbusiness/gotosocial/internal/oauth"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
"github.com/superseriousbusiness/gotosocial/internal/util"
)
@@ -161,13 +162,13 @@ func (p *processor) searchStatusByURI(ctx context.Context, authed *oauth.Auth, u
if resolve {
// This is a non-local status and we're allowed to resolve, so dereference it
- status, statusable, err := p.federator.GetRemoteStatus(ctx, authed.Account.Username, uri, true, true)
+ status, statusable, err := p.federator.GetRemoteStatus(transport.WithFastfail(ctx), authed.Account.Username, uri, true, true)
if err != nil {
return nil, fmt.Errorf("searchStatusByURI: error fetching remote status %q: %v", uriStr, err)
}
// Attempt to dereference the status thread while we are here
- p.federator.DereferenceRemoteThread(ctx, authed.Account.Username, uri, status, statusable)
+ p.federator.DereferenceRemoteThread(transport.WithFastfail(ctx), authed.Account.Username, uri, status, statusable)
}
return nil, nil
@@ -190,7 +191,7 @@ func (p *processor) searchAccountByURI(ctx context.Context, authed *oauth.Auth,
}
// we don't have it yet, try to find it remotely
- return p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ return p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: authed.Account.Username,
RemoteAccountID: uri,
Blocking: true,
@@ -209,7 +210,7 @@ func (p *processor) searchAccountByMention(ctx context.Context, authed *oauth.Au
}
// we don't have it yet, try to find it remotely
- return p.federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ return p.federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: authed.Account.Username,
RemoteAccountUsername: username,
RemoteAccountHost: domain,
diff --git a/internal/processing/util.go b/internal/processing/util.go
@@ -28,6 +28,7 @@ import (
"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
"github.com/superseriousbusiness/gotosocial/internal/id"
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
"github.com/superseriousbusiness/gotosocial/internal/util"
)
@@ -56,7 +57,7 @@ func GetParseMentionFunc(dbConn db.DB, federator federation.Federator) gtsmodel.
if originAccount.Domain == "" {
requestingUsername = originAccount.Username
}
- remoteAccount, err := federator.GetRemoteAccount(ctx, dereferencing.GetRemoteAccountParams{
+ remoteAccount, err := federator.GetRemoteAccount(transport.WithFastfail(ctx), dereferencing.GetRemoteAccountParams{
RequestingUsername: requestingUsername,
RemoteAccountUsername: username,
RemoteAccountHost: domain,
diff --git a/internal/transport/context.go b/internal/transport/context.go
@@ -0,0 +1,43 @@
+/*
+ GoToSocial
+ Copyright (C) 2021-2022 GoToSocial Authors admin@gotosocial.org
+
+ This program is free software: you can redistribute it and/or modify
+ it under the terms of the GNU Affero General Public License as published by
+ the Free Software Foundation, either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU Affero General Public License for more details.
+
+ You should have received a copy of the GNU Affero General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>.
+*/
+
+package transport
+
+import "context"
+
+// ctxkey is our own unique context key type to prevent setting outside package.
+type ctxkey string
+
+// fastfailkey is our unique context key to indicate fast-fail is enabled.
+var fastfailkey = ctxkey("ff")
+
+// WithFastfail returns a Context which indicates that any http requests made
+// with it should return after the first failed attempt, instead of retrying.
+//
+// This can be used to fail quickly when you're making an outgoing http request
+// inside the context of an incoming http request, and you want to be able to
+// provide a snappy response to the user, instead of retrying + backing off.
+func WithFastfail(parent context.Context) context.Context {
+ return context.WithValue(parent, fastfailkey, struct{}{})
+}
+
+// IsFastfail returns true if the given context was created by WithFastfail.
+func IsFastfail(ctx context.Context) bool {
+ _, ok := ctx.Value(fastfailkey).(struct{})
+ return ok
+}
diff --git a/internal/transport/context_test.go b/internal/transport/context_test.go
@@ -0,0 +1,34 @@
+/*
+ GoToSocial
+ Copyright (C) 2021-2022 GoToSocial Authors admin@gotosocial.org
+
+ This program is free software: you can redistribute it and/or modify
+ it under the terms of the GNU Affero General Public License as published by
+ the Free Software Foundation, either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU Affero General Public License for more details.
+
+ You should have received a copy of the GNU Affero General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>.
+*/
+
+package transport_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/superseriousbusiness/gotosocial/internal/transport"
+)
+
+func TestFastFailContext(t *testing.T) {
+ ctx := context.Background()
+ ctx = transport.WithFastfail(ctx)
+ if !transport.IsFastfail(ctx) {
+ t.Fatal("failed to set fast-fail context key")
+ }
+}
diff --git a/internal/transport/transport.go b/internal/transport/transport.go
@@ -23,6 +23,7 @@ import (
"crypto"
"crypto/x509"
"errors"
+ "fmt"
"io"
"net/http"
"net/url"
@@ -33,25 +34,41 @@ import (
errorsv2 "codeberg.org/gruf/go-errors/v2"
"codeberg.org/gruf/go-kv"
"github.com/go-fed/httpsig"
- "github.com/superseriousbusiness/activity/pub"
"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
"github.com/superseriousbusiness/gotosocial/internal/httpclient"
"github.com/superseriousbusiness/gotosocial/internal/log"
)
-// Transport wraps the pub.Transport interface with some additional functionality for fetching remote media.
+// Transport implements the pub.Transport interface with some additional functionality for fetching remote media.
//
// Since the transport has the concept of 'shortcuts' for fetching data locally rather than remotely, it is
// not *always* the case that calling a Transport function does an http call, but it usually will for remote
// hosts or resources for which a shortcut isn't provided by the transport controller (also in this package).
+//
+// For any of the transport functions, if a Fastfail context is passed in as the first parameter, the function
+// will return after the first transport failure, instead of retrying + backing off.
type Transport interface {
- pub.Transport
+ /*
+ POST functions
+ */
+
+ // Deliver sends an ActivityStreams object.
+ Deliver(ctx context.Context, b []byte, to *url.URL) error
+ // BatchDeliver sends an ActivityStreams object to multiple recipients.
+ BatchDeliver(ctx context.Context, b []byte, recipients []*url.URL) error
+
+ /*
+ GET functions
+ */
+
+ // Dereference fetches the ActivityStreams object located at this IRI with a GET request.
+ Dereference(ctx context.Context, iri *url.URL) ([]byte, error)
// DereferenceMedia fetches the given media attachment IRI, returning the reader and filesize.
DereferenceMedia(ctx context.Context, iri *url.URL) (io.ReadCloser, int64, error)
// DereferenceInstance dereferences remote instance information, first by checking /api/v1/instance, and then by checking /.well-known/nodeinfo.
DereferenceInstance(ctx context.Context, iri *url.URL) (*gtsmodel.Instance, error)
// Finger performs a webfinger request with the given username and domain, and returns the bytes from the response body.
- Finger(ctx context.Context, targetUsername string, targetDomains string) ([]byte, error)
+ Finger(ctx context.Context, targetUsername string, targetDomain string) ([]byte, error)
}
// transport implements the Transport interface
@@ -106,6 +123,10 @@ func (t *transport) do(r *http.Request, signer func(*http.Request) error, retryO
return nil, errors.New("too many failed attempts")
}
+ // Check whether request should fast fail, we check this
+ // before loop as each context.Value() requires mutex lock.
+ fastFail := IsFastfail(r.Context())
+
// Start a log entry for this request
l := log.WithFields(kv.Fields{
{"pubKeyID", t.pubKeyID},
@@ -156,6 +177,9 @@ func (t *transport) do(r *http.Request, signer func(*http.Request) error, retryO
} else if errors.As(err, &x509.UnknownAuthorityError{}) {
// Unknown authority errors we do NOT recover from
return nil, err
+ } else if fastFail {
+ // on fast-fail, don't bother backoff/retry
+ return nil, fmt.Errorf("%w (fast fail)", err)
}
l.Errorf("backing off for %s after http request error: %v", backoff.String(), err)
