Async I/O support + ALPN #127

mzohreva · 2020-10-13T00:23:14Z

No description provided.

Pagten · 2020-10-20T09:45:28Z

I went through the PR and it looks good to me, but I don't feel qualified to approve the async-related changes since I hardly have experience with async in rust. @jethrogb are you planning to review this?

jethrogb · 2020-12-01T14:18:47Z

This will need significant changes after landing #128 which will come first

jethrogb · 2021-01-27T09:01:50Z

Wrong target branch for the PR (or wrong rebase target)

mzohreva · 2021-01-27T18:12:18Z

This should target v0.6 but 0.6 was master when I opened the PR I think. I'll close this PR for now.

mbedtls/src/ssl/async_session.rs

jethrogb · 2021-04-26T09:03:24Z

Can this be rebased on 0.8?

mzohreva · 2021-04-27T22:02:13Z

Can this be rebased on 0.8?

I think it would not be straightforward to rebase this. I'll have to spend some time and do that separately.
But I think we can/should land this in 0.7 since it's being used as is.

raoulstrackx · 2021-07-19T06:43:43Z

Is there any progress on this? It's blocking other work we need.

Taowyoo · 2023-03-13T15:35:47Z

bors try

Taowyoo · 2023-03-13T15:36:40Z

Yes, there are some tests failed
The verify_chainone needs some more effort. Working on it.

Also add the full test command to ct.sh (without it, tests execute)

zugzwang

The PR looks good to me, but I found misleading test behavior and provided a fix in #225 so that tokio tests actually run in 2.28.0 (see my comment).

Edit: I am not suggesting we update to 2.28.0 in this PR, only that we add the feature and the cargo command.
Edit 2: Finally, this PR can be updated to mbedtls-sys 2.28.0, see the continuation of this thread and #225

zugzwang · 2023-03-13T15:03:24Z

mbedtls/Cargo.toml

+[[test]]
+name = "async_session"
+path = "tests/async_session.rs"
+required-features = ["std", "threading", "tokio", "tokio/net", "tokio/io-util", "tokio/macros"]


Based on the below, I believe we should add tokio/rt to this list, and make sure the test actually runs in CI with cargo test --features=std,threading,tokio,tokio/net,tokio/io-util,tokio/macros,tokio/rt

Note 1: I did update to mbedtls-sys 2.28.0 to pick up #152 and being able to actually test. See the diff here (a PR to this PR)

Edit: see @Taowyoo's comment below. My comment above still applies, but not for the reasons below.

~~Note 2: For the following, added an assert!(false) to the very beginning of the client_server_test so it FAILS.~~

When I run cargo test test::client_server_test (no features), cargo says the test ran ok (I was expecting filtered out, or FAIL, so this is either bad tokio usage in the test suite, or very misleading cargo behavior (might be cargo, see Note above) ❌

~~When I run cargo test test::client_server_test --features=std,threading,tokio,tokio/net, cargo says the test ran ok (so, same as 1) ❌~~

When I run cargo test test::client_server_test --features=std,threading,tokio,tokio/net,tokio/io-util,tokio/macros (the features specified here), I finally get a compilation error saying The #[tokio::test] macro requires rt or rt-multi-thread. ✔️

~~When I run cargo test test::client_server_test --features=std,threading,tokio,tokio/net,tokio/io-util,tokio/macros,tokio/rt, the test is finally ran, resulting in the expected FAILURE. ✔️~~

~~When I add tokio/rt to the above list in line 95, the test is not run in my machine by any of the commands given in ct.sh, but they do parse it and report ok as if it had passed. ❌~~

Hi, I think the problem should because we have two tests both has name test::client_server_test , and the logic of using cargo test TEST_NAME is equals to run cargo test but also ask cargo to only run tests whose name includes TEST_NAME. And if you check the output of cargo test test::client_server_test there is no lines of Running tests/async_session.rs ...
The more accurate way is cargo test --test async_session
Then cargo will alert:

error: target `async_session` in package `mbedtls` requires the features: `std`, `threading`, `tokio`, `tokio/net`, `tokio/io-util`, `tokio/macros`, `tokio/rt` Consider enabling them by passing, e.g., `--features="std threading tokio tokio/net tokio/io-util tokio/macros tokio/rt"

Thanks for spotting this. I did not realize we have two tests with the same name. This explains all.

So, we still need to add tokio/rt to the required dependencies as per my comment, and also make sure the test runs in CI by putting the command in ct.sh

Yes.
I updated related CI script in #225

Thanks, I will review there.

bors · 2023-03-13T15:54:07Z

try

Build failed:

continuous-integration/travis-ci/push

Fix by updating certs with aws.com's certs chain

1. cfg macro related errors 2. wrong type error

Fix error by explicitly specify version for some deps

update ci to be similar to master version

zugzwang

Thanks for the PR.

After review and suggested changes addressed by @Taowyoo, this looks good to me.
I will give a tentative approval, subject to @mzohreva agreeing on #225.

I reviewed @Taowyoo's commits in #225. It includes:

Updating to mbedtls-sys 2.28.0
Make sure tests run on CI
Fix CI and build

The plan (again, subject to @mzohreva agreeing to #225) is to either

Approve definitely this PR, and decline it in favor of Merge async support branch to 0.7 rust-mbedtls #224 which contains all the backported commits that succeeded this PR (everything reviewed). Merge 224.
Incorporate the changes in Merge async support branch to 0.7 rust-mbedtls #224 here (already reviewed) and merge 127.

Taowyoo · 2023-03-15T00:45:40Z

Thanks for the PR.

After review and suggested changes addressed by @Taowyoo, this looks good to me. I will give a tentative approval, subject to @mzohreva agreeing on #225.

I reviewed @Taowyoo's commits in #225. It includes:

Updating to mbedtls-sys 2.28.0

Make sure tests run on CI

Fix CI and build

The plan (again, subject to @mzohreva agreeing to #225) is to either

Approve definitely this PR, and decline it in favor of Merge async support branch to 0.7 rust-mbedtls #224 which contains all the backported commits that succeeded this PR (everything reviewed). Merge 224.

Incorporate the changes in Merge async support branch to 0.7 rust-mbedtls #224 here (already reviewed) and merge 127.

@zugzwang
I think the second option is better, it will distinguish code change in this PR and #225 from #224.
So make it easy to review #224.

225: Update to mbedtls-sys 2.28, update test keys, and make sure async tests are run r=Taowyoo a=zugzwang Co-authored-by: francisco <[email protected]> Co-authored-by: Yuxiang Cao <[email protected]>

Taowyoo · 2023-03-15T03:58:23Z

bors r+

bors · 2023-03-15T04:35:24Z

Build succeeded:

continuous-integration/travis-ci/push

224: Merge async support branch to 0.7 rust-mbedtls r=Taowyoo a=Taowyoo This is the successor PR for #127 but with some more latest changes needed Co-authored-by: Raoul Strackx <[email protected]> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com> Co-authored-by: Yuxiang Cao <[email protected]>

mzohreva requested review from jethrogb and Pagten October 13, 2020 00:23

mzohreva force-pushed the mz/async-support branch from 9151be6 to 219f287 Compare January 27, 2021 00:30

mzohreva closed this Jan 27, 2021

jethrogb reopened this Jan 27, 2021

jethrogb changed the base branch from master to v0.6 January 27, 2021 18:14

Add support for non-blocking I/O and ALPN

b91d916

mzohreva force-pushed the mz/async-support branch from 219f287 to b91d916 Compare March 11, 2021 19:19

mzohreva changed the base branch from v0.6 to v0.7 March 11, 2021 19:19

Write the same data and len after ssl_write() returns would block

148b800

jethrogb reviewed Mar 16, 2021

View reviewed changes

mbedtls/src/ssl/async_session.rs Show resolved Hide resolved

Do the hash check only in debug mode

8b17c8f

AdrianCX mentioned this pull request Dec 9, 2021

Context<S>, generic over IO #171

Closed

Taowyoo mentioned this pull request Mar 10, 2023

Merge async support branch to 0.7 rust-mbedtls #224

Merged

Update mbedtls-sys to 2.28.0 and expired keys

3d44979

bors bot added a commit that referenced this pull request Mar 13, 2023

Try #127:

b08a15f

Add tokio/rt to required features for async tests

7abdc1b

Also add the full test command to ct.sh (without it, tests execute)

zugzwang reviewed Mar 13, 2023

View reviewed changes

Taowyoo added 2 commits March 13, 2023 22:34

test: fix verify_chain test

eb0220a

Fix by updating certs with aws.com's certs chain

fix: fix various error

70b51a5

1. cfg macro related errors 2. wrong type error

Taowyoo added 3 commits March 13, 2023 22:39

build: fix deps error

40741a5

Fix error by explicitly specify version for some deps

ci: update ci

2fc953f

update ci to be similar to master version

ci: better triggering async tests

8577c26

Taowyoo mentioned this pull request Mar 14, 2023

Update to mbedtls-sys 2.28, update test keys, and make sure async tests are run #225

Merged

build: sync "proc-macro2" to master version

ec58f80

zugzwang self-assigned this Mar 14, 2023

zugzwang approved these changes Mar 14, 2023

View reviewed changes

Taowyoo and others added 3 commits March 14, 2023 17:58

ci: simpler tokio features

884167a

build: fix deps error

f0c3caa

Merge #225

ae32aa4

225: Update to mbedtls-sys 2.28, update test keys, and make sure async tests are run r=Taowyoo a=zugzwang Co-authored-by: francisco <[email protected]> Co-authored-by: Yuxiang Cao <[email protected]>

bors bot merged commit 44da8d0 into v0.7 Mar 15, 2023

Taowyoo deleted the mz/async-support branch March 21, 2023 22:59

Taowyoo restored the mz/async-support branch March 21, 2023 23:00

Taowyoo deleted the mz/async-support branch July 12, 2023 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async I/O support + ALPN #127

Async I/O support + ALPN #127

mzohreva commented Oct 13, 2020

Pagten commented Oct 20, 2020

jethrogb commented Dec 1, 2020

jethrogb commented Jan 27, 2021 •

edited

Loading

mzohreva commented Jan 27, 2021

jethrogb commented Apr 26, 2021

mzohreva commented Apr 27, 2021

raoulstrackx commented Jul 19, 2021

Taowyoo commented Mar 13, 2023

Taowyoo commented Mar 13, 2023

zugzwang left a comment •

edited

Loading

zugzwang Mar 13, 2023 •

edited

Loading

Taowyoo Mar 14, 2023

zugzwang Mar 14, 2023

Taowyoo Mar 14, 2023

zugzwang Mar 14, 2023 •

edited

Loading

bors bot commented Mar 13, 2023

zugzwang left a comment

Taowyoo commented Mar 15, 2023 •

edited

Loading

Taowyoo commented Mar 15, 2023

bors bot commented Mar 15, 2023

Async I/O support + ALPN #127

Async I/O support + ALPN #127

Conversation

mzohreva commented Oct 13, 2020

Pagten commented Oct 20, 2020

jethrogb commented Dec 1, 2020

jethrogb commented Jan 27, 2021 • edited Loading

mzohreva commented Jan 27, 2021

jethrogb commented Apr 26, 2021

mzohreva commented Apr 27, 2021

raoulstrackx commented Jul 19, 2021

Taowyoo commented Mar 13, 2023

Taowyoo commented Mar 13, 2023

zugzwang left a comment • edited Loading

Choose a reason for hiding this comment

zugzwang Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Taowyoo Mar 14, 2023

Choose a reason for hiding this comment

zugzwang Mar 14, 2023

Choose a reason for hiding this comment

Taowyoo Mar 14, 2023

Choose a reason for hiding this comment

zugzwang Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

bors bot commented Mar 13, 2023

try

zugzwang left a comment

Choose a reason for hiding this comment

Taowyoo commented Mar 15, 2023 • edited Loading

Taowyoo commented Mar 15, 2023

bors bot commented Mar 15, 2023

jethrogb commented Jan 27, 2021 •

edited

Loading

zugzwang left a comment •

edited

Loading

zugzwang Mar 13, 2023 •

edited

Loading

zugzwang Mar 14, 2023 •

edited

Loading

Taowyoo commented Mar 15, 2023 •

edited

Loading