fix: Update to tokio 1.0 #909

untitaker · 2021-01-13T13:04:18Z

Also enable trust-dns for reqwest client.

The tokio upgrade brought a few transitive dependency updates,
specifically syn that then broke enumset. I updated enumset to a
fork that contains the fix. The fork is from the wasmer team, so seems
trustworthy enough. enumset has been released and it's no longer necessary

#skip-changelog

Also enable trust-dns for reqwest client. The tokio upgrade brought a few transitive dependency updates, specifically `syn` that then broke `enumset`. I updated enumset to a fork that contains the fix. The fork is from the wasmer team, so seems trustworthy enough.

Swatinem · 2021-01-13T13:20:47Z

relay-general/Cargo.toml

@@ -14,7 +14,8 @@ chrono = { version = "0.4.11", features = ["serde"] }
 cookie = { version = "0.12.0", features = ["percent-encode"] }
 debugid = { version = "0.7.2", features = ["serde"] }
 dynfmt = { version = "0.1.4", features = ["python", "curly"] }
-enumset = "1.0.1"
+# TODO: Move back to enumset once https://github.com/Lymia/enumset/pull/18 is merged
+wasmer_enumset = { version = "1.0.1" }


might as well define an alias so you minimize the diff in the code

the auto-derive has the crate name hardcoded so it doesn't actually work.

Enumset 1.0.3 has been released which should contain the fix.

Swatinem · 2021-01-13T13:21:40Z

Cargo.lock

+name = "bytes"
+version = "1.0.0"


might be a good idea to also check/update all other dependencies. since this now includes 0.4, 0.5 and 1

this comes from multiple tokio versions which comes from multiple hyper versions which comes from multiple reqwest versions

we can update sentry sdk to fix this but I would do that in a separate PR

We can't, since we mostly have to deal with the bytes we get from actix, which is still on 0.4. We can only resolve this once we upgrade the web layer.

Cargo.lock

jan-auer

@Swatinem it would be nice to create a timely release of the Rust SDK with new reqwest/hyper.

jan-auer · 2021-01-13T21:43:02Z

relay-server/src/actors/upstream.rs

@@ -402,6 +402,7 @@ impl UpstreamRelay {
                    .connect_timeout(config.http_connection_timeout())


I just noticed that you have rt-multi-thread enabled. This means that the runtime you create here is multi threaded, as opposed to the single thread that the actix client runs in. Can you restrict this to a single thread and test throughput?

While it makes sense to eventually multi-thread, let's please stick to the proven behavior so that upstream connections to not compete with event processors for resources.

In Symbolicator we do this, although in Relay we should be able to run with just the current_thread scheduler, too.

You're right. I wanted to preserve tokio 0.x behavior but actually the default ::new() was single-threaded in 0.x. So I committed your suggestion.

I cannot explain it but current_thread yields 503 in the healthcheck in tests. I believe it blocks the wrong thread for too long.

Based on that I believe it would be safest to defer merge of this PR until after the OSS release. The OSS release should get by fine with getaddrinfo DNS resolution and old tokio.

reqwest internally likely needs blocking I/O, so use rt-multi-thread and spawn with a single worker thread. This works in symbolicator and will preserve behavior.

Swatinem · 2021-01-14T08:37:57Z

it would be nice to create a timely release of the Rust SDK with new reqwest/hyper.

done. Please update ;-)

This reverts commit 3f7742c.

jan-auer · 2021-02-22T13:04:45Z

relay-server/Cargo.toml

@@ -57,15 +57,15 @@ relay-general = { path = "../relay-general" }
 relay-log = { path = "../relay-log" }
 relay-quotas = { path = "../relay-quotas" }
 relay-redis = { path = "../relay-redis" }
-reqwest = { version = "0.10.8", features = ["gzip", "stream"] }
+reqwest = { version = "0.11.0", features = ["gzip", "stream", "trust-dns"] }


Let's upgrade to 0.11.1 to get some of the proxy fixes in.

ran into native-tls issue here: seanmonstar/reqwest#1181

* master: (23 commits) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) feat(protocol): Add NSError to mechanism (#925) meta: Fix changelog meta(readme): Document development config for processing mode (#921) ...

* master: (42 commits) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) feat(protocol): Add NSError to mechanism (#925) meta: Fix changelog ...

* master: (120 commits) fix(protocol): Deny backslashes in release names (#904) ref(server): Extract dynamic sampling functionality into own crate (#940) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) ...

* master: test(general): Introduce macros for traversing Annotated (#826) fix(protocol): Deny backslashes in release names (#904) ref(server): Extract dynamic sampling functionality into own crate (#940) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930)

untitaker requested a review from a team January 13, 2021 13:04

Swatinem reviewed Jan 13, 2021

View reviewed changes

jan-auer approved these changes Jan 13, 2021

View reviewed changes

jan-auer reviewed Jan 13, 2021

View reviewed changes

use current_thread runtime

3f7742c

untitaker and others added 7 commits January 14, 2021 11:54

Revert "use current_thread runtime"

8e83a1a

This reverts commit 3f7742c.

update sentry SDK

268fdff

Merge remote-tracking branch 'origin/master' into fix/tokio-update

1d3d891

singlethreaded

6f18e59

revert enumset

d8f7c6d

get rid of false-positive enumset warning

f823437

clean up toml

6259e61

jan-auer reviewed Feb 22, 2021

View reviewed changes

untitaker and others added 2 commits February 22, 2021 14:21

upgrade reqwest

c6c38c8

build: Update lockfile

941dbc4

jan-auer approved these changes Feb 22, 2021

View reviewed changes

untitaker merged commit 4409c27 into master Feb 22, 2021

untitaker deleted the fix/tokio-update branch February 22, 2021 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Update to tokio 1.0 #909

fix: Update to tokio 1.0 #909

untitaker commented Jan 13, 2021 •

edited

Loading

Swatinem Jan 13, 2021

untitaker Jan 13, 2021

jan-auer Jan 29, 2021

Swatinem Jan 13, 2021

untitaker Jan 13, 2021

jan-auer Jan 13, 2021

jan-auer left a comment

jan-auer Jan 13, 2021

jan-auer Jan 13, 2021

untitaker Jan 14, 2021

jan-auer Jan 14, 2021

Swatinem commented Jan 14, 2021

jan-auer Feb 22, 2021

untitaker Feb 22, 2021

untitaker Feb 22, 2021

		@@ -402,6 +402,7 @@ impl UpstreamRelay {
		.connect_timeout(config.http_connection_timeout())

fix: Update to tokio 1.0 #909

fix: Update to tokio 1.0 #909

Conversation

untitaker commented Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swatinem commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Jan 13, 2021 •

edited

Loading