Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Send and process ACK-ECN #1

Closed
wants to merge 18 commits into from

Conversation

larseggert
Copy link

@larseggert larseggert commented Feb 14, 2024

Most of the remaining bits from mozilla#1495

Depends on mozilla#1604, will retarget this at mozilla/neqo once that has landed

larseggert and others added 5 commits February 14, 2024 16:22
Most of the remaining bits from mozilla#1495

Depends on mozilla#1604
…a#1668)

Removes `fn` `set_sendorder` and `set_fairness` on `SendStream` trait.

The `SendStream` trait is not publicly exposed. Neither `set_sendorder` nor
`set_fairness` is called within `neqo-http3`.

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Copy link
Owner

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have rebased the commits onto latest mozilla#1604 and added a couple of smaller patches.

I have run this through the ECN QUIC Interop testcase. As of now, the server receives ECT0 from the client, but the server fails to properly send ECT0 to the client. This is due to wrong ECN handling of IPv4-Mapped IPv6 addresses. I will have a patch coming shortly. With the patch the ECN testcase between a neqo client and server succeeds.

//CC @larseggert to prevent duplicate work.

neqo-transport/src/connection/mod.rs Outdated Show resolved Hide resolved
neqo-transport/src/tracking.rs Outdated Show resolved Hide resolved
neqo-transport/src/tracking.rs Show resolved Hide resolved
neqo-client/src/main.rs Outdated Show resolved Hide resolved
mxinden and others added 4 commits February 22, 2024 08:46
`neqo-interop` is replaced by `neqo-client` and `neqo-server` and thus unused.
* feat: update to Rust 2021 edition

See transition docs:

https://doc.rust-lang.org/edition-guide/editions/transitioning-an-existing-project-to-a-new-edition.html

The described `cargo fix --edition` does not yield any suggestions.

* fix: clippy

---------

Co-authored-by: Martin Thomson <mt@lowentropy.net>
* Add some more doc-stuff

* Get rid of redundant imports

No more use of `use super::*` in tests.
Avoid importing things that are in the rust prelude.
For unit enum default variants, derive `Default` and use the `#[default]` attribute.

See https://doc.rust-lang.org/std/default/trait.Default.html#enums for details.

Co-authored-by: Martin Thomson <mt@lowentropy.net>
@mxinden
Copy link
Owner

mxinden commented Feb 23, 2024

I have run this through the ECN QUIC Interop testcase. As of now, the server receives ECT0 from the client, but the server fails to properly send ECT0 to the client. This is due to wrong ECN handling of IPv4-Mapped IPv6 addresses. I will have a patch coming shortly. With the patch the ECN testcase between a neqo client and server succeeds.

Fixed with quinn-rs/quinn#1765. Updated in mozilla#1604 via mozilla@a0d3093.

@mxinden
Copy link
Owner

mxinden commented Feb 23, 2024

For convenience, I pushed my changes to https://github.com/mxinden/neqo/tree/feat-more-ecn-2. It includes:

mxinden and others added 3 commits February 26, 2024 10:41
Enables workflows to run on pull requests within the merge queue.
* Benchmark tweaks

1. Use NSS Release
2. Add workflow_dispatch
3. Move some env values up in the file

* YAML corrections

Co-authored-by: Lars Eggert <lars@eggert.org>
Signed-off-by: Martin Thomson <mt@lowentropy.net>

* Build NSS optimized

---------

Signed-off-by: Martin Thomson <mt@lowentropy.net>
Co-authored-by: Lars Eggert <lars@eggert.org>
* Initial commit

* refactor(server): replace mio with tokio

* Move ready logic into fn

* Extend expect docs

* Restrict tokio features

* Only process datagram once

* Remove superfluous pub

* fmt

* Fix send path

* Fix receive path

* Instantiate socket state once

* Fix busy loop

* Have neqo-client use quinn-udp

* Add TODO

* Await writable

* Unify tx and rx

* Introduce wrapper type Socket

* Move bind to common

* Check if datagram was sent as a whole and avoid allocation

* Make into_data pub(crate)

* Refactor send

* Reference issue

* Remove debugs

* Fix test

* Reduce diff

* Reduce diff

* Pin quinn-udp to rev

* Address clippy lints

* fmt

* fmt

* clippy

* clippy

* Pass None ttl

Not yet supported by quinn-udp.

* Debug race condition on Windows

* Debug windows failure

* Have receiver use random port

* Test with Ect1 instead of Ce

Windows does not allow setting Ce:

> Your application isn't allowed to specify the Congestion Encountered (CE) code point when sending datagrams. The send will return with error WSAEINVAL.

https://learn.microsoft.com/en-us/windows/win32/winsock/winsock-ecn

* Revert "Debug windows failure"

This reverts commit e9ac36c.

* Revert "Debug race condition on Windows"

This reverts commit 6f330d3.

* Fold tos_tx

* Add reason to clippy lint ignore

* fix: include quinn-udp IPv4-mapped IPv6 patch

quinn-rs/quinn#1765

---------

Co-authored-by: Lars Eggert <lars@eggert.org>
@larseggert larseggert changed the base branch from quinn-udp to main February 26, 2024 07:12
@larseggert larseggert changed the base branch from main to feat-more-ecn-2 February 26, 2024 07:13
@larseggert
Copy link
Author

See mozilla#1678

@larseggert larseggert closed this Feb 26, 2024
mxinden pushed a commit that referenced this pull request Jul 1, 2024
They seem to have a race condition that makes them crash.

I wasn't able to figure out why they crash, this seems to happen
inside NSS:
```
Process 47677 stopped
* thread #2, name = 'init_twice_withdb', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x000000019b4994f8 libsystem_pthread.dylib`pthread_mutex_lock + 12
libsystem_pthread.dylib`pthread_mutex_lock:
->  0x19b4994f8 <+12>: ldr    x8, [x0]
    0x19b4994fc <+16>: mov    w9, #0x545a ; =21594
    0x19b499500 <+20>: movk   w9, #0x4d55, lsl mozilla#16
    0x19b499504 <+24>: cmp    x8, x9
(lldb) up
frame #1: 0x0000000100802f58 libnspr4.dylib`PR_Lock + 20
libnspr4.dylib`PR_Lock:
->  0x100802f58 <+20>: bl     0x10080c0c0    ; symbol stub for: pthread_self
    0x100802f5c <+24>: str    x0, [x19, #0xb8]
    0x100802f60 <+28>: mov    w8, #0x1 ; =1
    0x100802f64 <+32>: str    w8, [x19, #0xb0]
(lldb) up
frame #2: 0x00000001007fc188 libnspr4.dylib`PR_CallOnce + 56
libnspr4.dylib`PR_CallOnce:
->  0x1007fc188 <+56>: ldr    w23, [x19]
    0x1007fc18c <+60>: ldr    w20, [x19, #0x8]
    0x1007fc190 <+64>: ldr    x0, [x22, #0x430]
    0x1007fc194 <+68>: bl     0x100802f74    ; PR_Unlock
(lldb) up
frame mozilla#3: 0x000000010066f634 libnss3.dylib`nss_Init + 112
libnss3.dylib`nss_Init:
->  0x10066f634 <+112>: cbz    w0, 0x10066f640 ; <+124>
    0x10066f638 <+116>: mov    w19, #-0x1 ; =-1
    0x10066f63c <+120>: b      0x10066f968    ; <+932>
    0x10066f640 <+124>: stp    x21, x22, [x29, #-0x80]
(lldb) up
frame mozilla#4: 0x000000010066ff44 libnss3.dylib`NSS_Initialize + 100
libnss3.dylib`NSS_Initialize:
->  0x10066ff44 <+100>: ldp    x29, x30, [sp, #0x40]
    0x10066ff48 <+104>: add    sp, sp, #0x50
    0x10066ff4c <+108>: ret

libnss3.dylib`NSS_InitContext:
    0x10066ff50 <+0>:   sub    sp, sp, #0x60
(lldb) up
frame mozilla#5: 0x00000001000035fc init-d4e9f02b33e50e46`init::init_twice_withdb::h34013f715e9b4f6e at init.rs:65:9
   62  	    let pathstr = path.to_str().unwrap();
   63  	    let dircstr = CString::new(pathstr).unwrap();
   64  	    unsafe {
-> 65  	        nss::NSS_Initialize(
   66  	            dircstr.as_ptr(),
   67  	            empty.as_ptr(),
   68  	            empty.as_ptr(),
```
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.

3 participants