Skip to content

asyncnet ssl overhaul#24896

Merged
Araq merged 7 commits intonim-lang:develfrom
nitely:asyncnet_overhaul
Apr 29, 2025
Merged

asyncnet ssl overhaul#24896
Araq merged 7 commits intonim-lang:develfrom
nitely:asyncnet_overhaul

Conversation

@nitely
Copy link
Contributor

@nitely nitely commented Apr 20, 2025

Fixes #24895

  • Remove all bio handling
  • Remove all sendPendingSslData which only seems to make things work by chance
  • Wrap the client socket on acceptAddr (std/net does this)
  • Do the SSL handshake on accept (std/net does this)

The only concern is if addWrite/addRead works well on Windows.

@nitely nitely closed this Apr 21, 2025
@Araq
Copy link
Member

Araq commented Apr 21, 2025

What was the insight gained here?

@nitely nitely reopened this Apr 21, 2025
@nitely nitely force-pushed the asyncnet_overhaul branch from 9ee1feb to 903e223 Compare April 21, 2025 17:32
@nitely
Copy link
Contributor Author

nitely commented Apr 21, 2025

The official guide for client and server use the SSL_* API family, which underneath use the BIO_* APIs. BIO_* allows to handle the I/O (send/recv) in application code. SSL_* handles the I/O internally, and returns WANT_READ/WANT_WRITE (note a ssl_read may want to write and ssl_write may want to read, because of renegotiation) so the socket can be awaited to become readable/writeable and repeat the operation.

Nothing I found in the docs hints one should mix BIO_* I/O with SSL_* like we did; although that does not mean this is not valid, it does not look like the way to do it. If using BIO_ were required, I think the way would be copying what SSL_[READ, WRITE, CONNECT, ACCEPT] do into the application code, and get rid of all the SSL_*, which does not seem future-proof/wise either. The only example I found of using BIO is this but it's using a blocking socket, which takes care of things like renegotiation.

Looking at std/net, there is almost a 1:1 map to std/asyncnet after these changes wrt error handling.

@nitely nitely force-pushed the asyncnet_overhaul branch from 2c15f6c to 2b5f267 Compare April 28, 2025 19:38
@nitely nitely marked this pull request as ready for review April 28, 2025 21:48
@Araq Araq merged commit 8518cf0 into nim-lang:devel Apr 29, 2025
18 checks passed
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 8518cf0

Hint: mm: orc; opt: speed; options: -d:release
179420 lines; 9.178s; 651.219MiB peakmem

narimiran pushed a commit that referenced this pull request May 5, 2025
Fixes #24895

- Remove all  bio handling
- Remove all `sendPendingSslData` which only seems to make things work
by chance
- Wrap the client socket on `acceptAddr` (std/net does this)
- Do the SSL handshake on accept (std/net does this)

The only concern is if addWrite/addRead works well on Windows.

(cherry picked from commit 8518cf0)
Araq pushed a commit that referenced this pull request Jul 1, 2025
…n devel (#25024)

Fixes #25023

Revert the acceptAddr #24896 change. SSL_accept is no longer explicitly
called.
This was referenced Jul 7, 2025
narimiran pushed a commit that referenced this pull request Jul 8, 2025
…n devel (#25024)

Fixes #25023

Revert the acceptAddr #24896 change. SSL_accept is no longer explicitly
called.

(cherry picked from commit fbdc9a4)
Araq pushed a commit that referenced this pull request Jul 10, 2025
revert #24896

Partially reverting #24896 in #25024 broke CI. So better revert it
completely so the CI is green. I'll investigate the issue later.
narimiran pushed a commit that referenced this pull request Jul 10, 2025
revert #24896

Partially reverting #24896 in #25024 broke CI. So better revert it
completely so the CI is green. I'll investigate the issue later.

(cherry picked from commit 08642ff)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Async SSL hangs when send and recv concurrently

2 participants

Comments