From d8b19c857ffa3f8818ef7f16fa2cda6ba9324430 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 20 Sep 2022 17:17:44 +0100 Subject: [PATCH 1/5] HTTP connection keepalives (#2730) Beforehand we disabled HTTP keepalives to prevent ambient system resources from being used by excess idle connections. Now that we've fixed some bugs in the federation API and device list updater, this situation is now much better and we don't open so many remote connections anyway. Keepalives allow us to not have to handshake TLS so often (which is quite expensive) and reusing an idle connection is much faster than having to open a new one. This can help with response times when talking to remote federated servers. This PR also adds a new option to disable keepalives if needed: ``` # Disable HTTP keepalives, which also prevents connection reuse. Dendrite will typically # keep HTTP connections open to remote hosts for 5 minutes as they can be reused much # more quickly than opening new connections each time. Disabling keepalives will close # HTTP connections immediately after a successful request but may result in more CPU and # memory being used on TLS handshakes for each new connection instead. disable_http_keepalives: false ``` --- dendrite-sample.monolith.yaml | 7 +++++++ dendrite-sample.polylith.yaml | 7 +++++++ setup/base/base.go | 1 + setup/config/config_federationapi.go | 6 ++++++ 4 files changed, 21 insertions(+) diff --git a/dendrite-sample.monolith.yaml b/dendrite-sample.monolith.yaml index 195d11610..f1758f54d 100644 --- a/dendrite-sample.monolith.yaml +++ b/dendrite-sample.monolith.yaml @@ -212,6 +212,13 @@ federation_api: # enable this option in production as it presents a security risk! disable_tls_validation: false + # Disable HTTP keepalives, which also prevents connection reuse. Dendrite will typically + # keep HTTP connections open to remote hosts for 5 minutes as they can be reused much + # more quickly than opening new connections each time. Disabling keepalives will close + # HTTP connections immediately after a successful request but may result in more CPU and + # memory being used on TLS handshakes for each new connection instead. + disable_http_keepalives: false + # Perspective keyservers to use as a backup when direct key fetches fail. This may # be required to satisfy key requests for servers that are no longer online when # joining some rooms. diff --git a/dendrite-sample.polylith.yaml b/dendrite-sample.polylith.yaml index 65d60e150..97d10825f 100644 --- a/dendrite-sample.polylith.yaml +++ b/dendrite-sample.polylith.yaml @@ -219,6 +219,13 @@ federation_api: # enable this option in production as it presents a security risk! disable_tls_validation: false + # Disable HTTP keepalives, which also prevents connection reuse. Dendrite will typically + # keep HTTP connections open to remote hosts for 5 minutes as they can be reused much + # more quickly than opening new connections each time. Disabling keepalives will close + # HTTP connections immediately after a successful request but may result in more CPU and + # memory being used on TLS handshakes for each new connection instead. + disable_http_keepalives: false + # Perspective keyservers to use as a backup when direct key fetches fail. This may # be required to satisfy key requests for servers that are no longer online when # joining some rooms. diff --git a/setup/base/base.go b/setup/base/base.go index 0a34523e7..0c7b222d0 100644 --- a/setup/base/base.go +++ b/setup/base/base.go @@ -373,6 +373,7 @@ func (b *BaseDendrite) CreateFederationClient() *gomatrixserverlib.FederationCli opts := []gomatrixserverlib.ClientOption{ gomatrixserverlib.WithTimeout(time.Minute * 5), gomatrixserverlib.WithSkipVerify(b.Cfg.FederationAPI.DisableTLSValidation), + gomatrixserverlib.WithKeepAlives(!b.Cfg.FederationAPI.DisableHTTPKeepalives), } if b.Cfg.Global.DNSCache.Enabled { opts = append(opts, gomatrixserverlib.WithDNSCache(b.DNSCache)) diff --git a/setup/config/config_federationapi.go b/setup/config/config_federationapi.go index d3a4b2b67..0f853865f 100644 --- a/setup/config/config_federationapi.go +++ b/setup/config/config_federationapi.go @@ -22,6 +22,11 @@ type FederationAPI struct { // on remote federation endpoints. This is not recommended in production! DisableTLSValidation bool `yaml:"disable_tls_validation"` + // DisableHTTPKeepalives prevents Dendrite from keeping HTTP connections + // open for reuse for future requests. Connections will be closed quicker + // but we may spend more time on TLS handshakes instead. + DisableHTTPKeepalives bool `yaml:"disable_http_keepalives"` + // Perspective keyservers, to use as a backup when direct key fetch // requests don't succeed KeyPerspectives KeyPerspectives `yaml:"key_perspectives"` @@ -39,6 +44,7 @@ func (c *FederationAPI) Defaults(opts DefaultOpts) { } c.FederationMaxRetries = 16 c.DisableTLSValidation = false + c.DisableHTTPKeepalives = false if opts.Generate { c.KeyPerspectives = KeyPerspectives{ { From 61a34d7cfb840bcf599218bf7c2559b6b8f95998 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 22 Sep 2022 11:21:43 +0100 Subject: [PATCH 2/5] Bump commonmarker from 0.23.4 to 0.23.6 in /docs (#2731) Bumps [commonmarker](https://github.com/gjtorikian/commonmarker) from 0.23.4 to 0.23.6.
Release notes

Sourced from commonmarker's releases.

v0.23.6

What's Changed

This release includes two updates from the upstream cmark-gfm library, namely:

Changelog

Sourced from commonmarker's changelog.

Changelog

Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=commonmarker&package-manager=bundler&previous-version=0.23.4&new-version=0.23.6)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/matrix-org/dendrite/network/alerts).
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- docs/Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/Gemfile.lock b/docs/Gemfile.lock index 88d33ae84..bc73df728 100644 --- a/docs/Gemfile.lock +++ b/docs/Gemfile.lock @@ -14,7 +14,7 @@ GEM execjs coffee-script-source (1.11.1) colorator (1.1.0) - commonmarker (0.23.4) + commonmarker (0.23.6) concurrent-ruby (1.1.10) dnsruby (1.61.9) simpleidn (~> 0.1) From 852d856db85abb7dacf3d323b0bcb355736bc656 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Thu, 22 Sep 2022 13:55:40 +0100 Subject: [PATCH 3/5] Update embedded NATS Server to v2.9.1-beta1 (as this includes a fix that prevents high CPU usage after Dendrite startup) --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index bd401ca65..48f6a1a3f 100644 --- a/go.mod +++ b/go.mod @@ -26,7 +26,7 @@ require ( github.com/matrix-org/pinecone v0.0.0-20220915154206-df85cb5026fc github.com/matrix-org/util v0.0.0-20200807132607-55161520e1d4 github.com/mattn/go-sqlite3 v1.14.15 - github.com/nats-io/nats-server/v2 v2.9.0 + github.com/nats-io/nats-server/v2 v2.9.1-0.20220920152220-52d7b481c4b5 github.com/nats-io/nats.go v1.17.0 github.com/neilalexander/utp v0.1.1-0.20210727203401-54ae7b1cd5f9 github.com/nfnt/resize v0.0.0-20180221191011-83c6a9932646 diff --git a/go.sum b/go.sum index b180ea46f..73c20e4d1 100644 --- a/go.sum +++ b/go.sum @@ -422,8 +422,8 @@ github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRW github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= github.com/nats-io/jwt/v2 v2.3.0 h1:z2mA1a7tIf5ShggOFlR1oBPgd6hGqcDYsISxZByUzdI= github.com/nats-io/jwt/v2 v2.3.0/go.mod h1:0tqz9Hlu6bCBFLWAASKhE5vUA4c24L9KPUUgvwumE/k= -github.com/nats-io/nats-server/v2 v2.9.0 h1:DLWu+7/VgGOoChcDKytnUZPAmudpv7o/MhKmNrnH1RE= -github.com/nats-io/nats-server/v2 v2.9.0/go.mod h1:BWKY6217RvhI+FDoOLZ2BH+hOC37xeKRBlQ1Lz7teKI= +github.com/nats-io/nats-server/v2 v2.9.1-0.20220920152220-52d7b481c4b5 h1:G/YGSXcJ2bUofD8Ts49it4VNezaJLQldI6fZR+wIUts= +github.com/nats-io/nats-server/v2 v2.9.1-0.20220920152220-52d7b481c4b5/go.mod h1:BWKY6217RvhI+FDoOLZ2BH+hOC37xeKRBlQ1Lz7teKI= github.com/nats-io/nats.go v1.17.0 h1:1jp5BThsdGlN91hW0k3YEfJbfACjiOYtUiLXG0RL4IE= github.com/nats-io/nats.go v1.17.0/go.mod h1:BPko4oXsySz4aSWeFgOHLZs3G4Jq4ZAyE6/zMCxRT6w= github.com/nats-io/nkeys v0.3.0 h1:cgM5tL53EvYRU+2YLXIK0G2mJtK12Ft9oeooSZMA2G8= From 0ddfb0cad4929e38d1995d8983e818432a942c81 Mon Sep 17 00:00:00 2001 From: Till <2353100+S7evinK@users.noreply.github.com> Date: Thu, 22 Sep 2022 15:53:15 +0200 Subject: [PATCH 4/5] Tweak `InsertMigration` to avoid logging (#2720) We'd still produce logs in Postgres when trying to insert a migration we already ran. This should stop us from creating those log entries. --- federationapi/federationapi_keys_test.go | 10 +++- federationapi/federationapi_test.go | 8 +-- internal/sqlutil/migrate.go | 62 ++++++++++++++---------- internal/sqlutil/migrate_test.go | 42 +++++++++++++--- 4 files changed, 82 insertions(+), 40 deletions(-) diff --git a/federationapi/federationapi_keys_test.go b/federationapi/federationapi_keys_test.go index 4469da35b..85cc43aa5 100644 --- a/federationapi/federationapi_keys_test.go +++ b/federationapi/federationapi_keys_test.go @@ -12,12 +12,13 @@ import ( "testing" "time" + "github.com/matrix-org/gomatrixserverlib" + "github.com/matrix-org/dendrite/federationapi/api" "github.com/matrix-org/dendrite/federationapi/routing" "github.com/matrix-org/dendrite/internal/caching" "github.com/matrix-org/dendrite/setup/base" "github.com/matrix-org/dendrite/setup/config" - "github.com/matrix-org/gomatrixserverlib" ) type server struct { @@ -86,7 +87,12 @@ func TestMain(m *testing.M) { cfg.Global.JetStream.StoragePath = config.Path(d) cfg.Global.KeyID = serverKeyID cfg.Global.KeyValidityPeriod = s.validity - cfg.FederationAPI.Database.ConnectionString = config.DataSource("file::memory:") + f, err := os.CreateTemp(d, "federation_keys_test*.db") + if err != nil { + return -1 + } + defer f.Close() + cfg.FederationAPI.Database.ConnectionString = config.DataSource("file:" + f.Name()) s.config = &cfg.FederationAPI // Create a transport which redirects federation requests to diff --git a/federationapi/federationapi_test.go b/federationapi/federationapi_test.go index 15f7a6840..e923143a7 100644 --- a/federationapi/federationapi_test.go +++ b/federationapi/federationapi_test.go @@ -10,6 +10,10 @@ import ( "testing" "time" + "github.com/matrix-org/gomatrix" + "github.com/matrix-org/gomatrixserverlib" + "github.com/nats-io/nats.go" + "github.com/matrix-org/dendrite/federationapi" "github.com/matrix-org/dendrite/federationapi/api" "github.com/matrix-org/dendrite/federationapi/internal" @@ -20,9 +24,6 @@ import ( "github.com/matrix-org/dendrite/setup/jetstream" "github.com/matrix-org/dendrite/test" "github.com/matrix-org/dendrite/test/testrig" - "github.com/matrix-org/gomatrix" - "github.com/matrix-org/gomatrixserverlib" - "github.com/nats-io/nats.go" ) type fedRoomserverAPI struct { @@ -271,7 +272,6 @@ func TestRoomsV3URLEscapeDoNot404(t *testing.T) { cfg.Global.ServerName = gomatrixserverlib.ServerName("localhost") cfg.Global.PrivateKey = privKey cfg.Global.JetStream.InMemory = true - cfg.FederationAPI.Database.ConnectionString = config.DataSource("file::memory:") base := base.NewBaseDendrite(cfg, "Monolith") keyRing := &test.NopJSONVerifier{} // TODO: This is pretty fragile, as if anything calls anything on these nils this test will break. diff --git a/internal/sqlutil/migrate.go b/internal/sqlutil/migrate.go index b6a8b1f25..a66a75826 100644 --- a/internal/sqlutil/migrate.go +++ b/internal/sqlutil/migrate.go @@ -49,12 +49,13 @@ type Migration struct { Down func(ctx context.Context, txn *sql.Tx) error } -// Migrator +// Migrator contains fields required to run migrations. type Migrator struct { db *sql.DB migrations []Migration knownMigrations map[string]struct{} mutex *sync.Mutex + insertStmt *sql.Stmt } // NewMigrator creates a new DB migrator. @@ -82,35 +83,26 @@ func (m *Migrator) AddMigrations(migrations ...Migration) { // Up executes all migrations in order they were added. func (m *Migrator) Up(ctx context.Context) error { - var ( - err error - dendriteVersion = internal.VersionString() - ) // ensure there is a table for known migrations executedMigrations, err := m.ExecutedMigrations(ctx) if err != nil { return fmt.Errorf("unable to create/get migrations: %w", err) } - + // ensure we close the insert statement, as it's not needed anymore + defer m.close() return WithTransaction(m.db, func(txn *sql.Tx) error { for i := range m.migrations { - now := time.Now().UTC().Format(time.RFC3339) migration := m.migrations[i] // Skip migration if it was already executed if _, ok := executedMigrations[migration.Version]; ok { continue } logrus.Debugf("Executing database migration '%s'", migration.Version) - err = migration.Up(ctx, txn) - if err != nil { + + if err = migration.Up(ctx, txn); err != nil { return fmt.Errorf("unable to execute migration '%s': %w", migration.Version, err) } - _, err = txn.ExecContext(ctx, insertVersionSQL, - migration.Version, - now, - dendriteVersion, - ) - if err != nil { + if err = m.insertMigration(ctx, txn, migration.Version); err != nil { return fmt.Errorf("unable to insert executed migrations: %w", err) } } @@ -118,6 +110,23 @@ func (m *Migrator) Up(ctx context.Context) error { }) } +func (m *Migrator) insertMigration(ctx context.Context, txn *sql.Tx, migrationName string) error { + if m.insertStmt == nil { + stmt, err := m.db.Prepare(insertVersionSQL) + if err != nil { + return fmt.Errorf("unable to prepare insert statement: %w", err) + } + m.insertStmt = stmt + } + stmt := TxStmtContext(ctx, txn, m.insertStmt) + _, err := stmt.ExecContext(ctx, + migrationName, + time.Now().Format(time.RFC3339), + internal.VersionString(), + ) + return err +} + // ExecutedMigrations returns a map with already executed migrations in addition to creating the // migrations table, if it doesn't exist. func (m *Migrator) ExecutedMigrations(ctx context.Context) (map[string]struct{}, error) { @@ -146,19 +155,20 @@ func (m *Migrator) ExecutedMigrations(ctx context.Context) (map[string]struct{}, // inserts a migration given their name to the database. // This should only be used when manually inserting migrations. func InsertMigration(ctx context.Context, db *sql.DB, migrationName string) error { - _, err := db.ExecContext(ctx, createDBMigrationsSQL) + m := NewMigrator(db) + defer m.close() + existingMigrations, err := m.ExecutedMigrations(ctx) if err != nil { - return fmt.Errorf("unable to create db_migrations: %w", err) + return err } - _, err = db.ExecContext(ctx, insertVersionSQL, - migrationName, - time.Now().Format(time.RFC3339), - internal.VersionString(), - ) - // If the migration was already executed, we'll get a unique constraint error, - // return nil instead, to avoid unnecessary logging. - if IsUniqueConstraintViolationErr(err) { + if _, ok := existingMigrations[migrationName]; ok { return nil } - return err + return m.insertMigration(ctx, nil, migrationName) +} + +func (m *Migrator) close() { + if m.insertStmt != nil { + internal.CloseAndLogIfError(context.Background(), m.insertStmt, "unable to close insert statement") + } } diff --git a/internal/sqlutil/migrate_test.go b/internal/sqlutil/migrate_test.go index d8bcae196..5116237a1 100644 --- a/internal/sqlutil/migrate_test.go +++ b/internal/sqlutil/migrate_test.go @@ -7,9 +7,10 @@ import ( "reflect" "testing" + _ "github.com/mattn/go-sqlite3" + "github.com/matrix-org/dendrite/internal/sqlutil" "github.com/matrix-org/dendrite/test" - _ "github.com/mattn/go-sqlite3" ) var dummyMigrations = []sqlutil.Migration{ @@ -81,11 +82,12 @@ func Test_migrations_Up(t *testing.T) { } ctx := context.Background() - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - test.WithAllDatabases(t, func(t *testing.T, dbType test.DBType) { - conStr, close := test.PrepareDBConnectionString(t, dbType) - defer close() + test.WithAllDatabases(t, func(t *testing.T, dbType test.DBType) { + conStr, close := test.PrepareDBConnectionString(t, dbType) + defer close() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { driverName := "sqlite3" if dbType == test.DBTypePostgres { driverName = "postgres" @@ -107,6 +109,30 @@ func Test_migrations_Up(t *testing.T) { t.Errorf("expected: %+v, got %v", tt.wantResult, result) } }) - }) - } + } + }) +} + +func Test_insertMigration(t *testing.T) { + test.WithAllDatabases(t, func(t *testing.T, dbType test.DBType) { + conStr, close := test.PrepareDBConnectionString(t, dbType) + defer close() + driverName := "sqlite3" + if dbType == test.DBTypePostgres { + driverName = "postgres" + } + + db, err := sql.Open(driverName, conStr) + if err != nil { + t.Errorf("unable to open database: %v", err) + } + + if err := sqlutil.InsertMigration(context.Background(), db, "testing"); err != nil { + t.Fatalf("unable to insert migration: %s", err) + } + // Second insert should not return an error, as it was already executed. + if err := sqlutil.InsertMigration(context.Background(), db, "testing"); err != nil { + t.Fatalf("unable to insert migration: %s", err) + } + }) } From f40e2803276001cafd4d382c64a661d6ea740870 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Thu, 22 Sep 2022 14:54:25 +0100 Subject: [PATCH 5/5] Version 0.9.9 (#2732) Changelog and version bump. --- CHANGES.md | 21 +++++++++++++++++++++ internal/version.go | 2 +- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 79d2fe2e1..657ca1920 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,26 @@ # Changelog +## Dendrite 0.9.9 (2022-09-22) + +### Features + +* Dendrite will now try to keep HTTP connections open to remote federated servers for a few minutes after a request and attempt to reuse those connections where possible + * This should reduce the amount of time spent on TLS handshakes and often speed up requests to remote servers + * This new behaviour can be disabled with the `federation_api.disable_http_keepalives` option if needed +* A number of dependencies have been updated + +### Fixes + +* A bug where the roomserver did not correctly propagate rewritten room state to downstream components (like the federation API and sync API) has been fixed, which could cause issues when performing a federated join to a previously left room +* Event auth now correctly parses the `join_authorised_via_users_server` field in the membership event content +* Database migrations should no longer produce unique constraint errors at Dendrite startup +* The `origin` of device list updates should now be populated correctly +* Send-to-device messages will no longer be dropped if we fail to publish them to specific devices +* The roomserver query to find state after events will now always resolve state if there are multiple prev events +* The roomserver will now return no memberships if querying history visibility for an event which has no state snapshot +* The device list updater will now mark a device list as stale if a requesting device ID is not known +* Transactions sent to appservices should no longer have accidental duplicated transaction IDs (contributed by [tak-hntlabs](https://github.com/tak-hntlabs)) + ## Dendrite 0.9.8 (2022-09-12) ### Important diff --git a/internal/version.go b/internal/version.go index ef7b879c2..f9b101702 100644 --- a/internal/version.go +++ b/internal/version.go @@ -17,7 +17,7 @@ var build string const ( VersionMajor = 0 VersionMinor = 9 - VersionPatch = 8 + VersionPatch = 9 VersionTag = "" // example: "rc1" )