fix(proto): Avoid active_connections underflow#717
Merged
Conversation
Also adds a regression test.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/717/docs/noq/ Last updated: 2026-06-19T13:17:19Z |
matheus23
force-pushed
the
matheus23/active-connections
branch
from
June 19, 2026 13:14
35ad58a to
6e759e3
Compare
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5178.4 Mbps | 7976.3 Mbps | -35.1% | 95.5% / 100.0% |
| medium-concurrent | 5312.1 Mbps | 7860.8 Mbps | -32.4% | 97.3% / 153.0% |
| medium-single | 4425.7 Mbps | 4749.6 Mbps | -6.8% | 97.3% / 151.0% |
| small-concurrent | 3893.3 Mbps | 5212.9 Mbps | -25.3% | 94.0% / 103.0% |
| small-single | 3561.9 Mbps | 4757.5 Mbps | -25.1% | 92.1% / 102.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3009.6 Mbps | N/A | N/A |
| lan | 782.4 Mbps | N/A | N/A |
| wan | 83.8 Mbps | N/A | N/A |
Summary
noq is 26.8% slower on average
6e759e37de31625a6930adcfa195f6e2da255a9e - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5494.7 Mbps | 7799.8 Mbps | -29.6% | 96.0% / 97.9% |
| medium-concurrent | 5277.8 Mbps | 7650.3 Mbps | -31.0% | 96.2% / 97.8% |
| medium-single | 3846.6 Mbps | 4749.1 Mbps | -19.0% | 96.2% / 98.2% |
| small-concurrent | 3711.3 Mbps | 5173.5 Mbps | -28.3% | 96.1% / 98.9% |
| small-single | 3513.6 Mbps | 4836.3 Mbps | -27.4% | 96.0% / 98.1% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3100.6 Mbps | 3799.2 Mbps | -18.4% |
| lan | 782.3 Mbps | 810.4 Mbps | -3.5% |
| lossy | 69.8 Mbps | 55.9 Mbps | +25.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 26.0% slower on average
flub
approved these changes
Jun 22, 2026
|
@matheus23 I just ran into this scenario. Thanks for fixing. Do you have a timeline when this will end up in a release? |
Member
Author
|
Plan is Monday. But let's tentatively say next week :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes an underflow panic on decrementing
active_connectionsin the noq endpoint state.This occurred when the API was used in this order:
Endpoint::acceptand captures theIncomingEndpoint::closeIncoming::accepton the captured incomingThe reason for this was that incrementing and decrementing the
active_connectionswas asymmetrical.We would increment
active_connectionsonly if the endpoint wasn't closed yet. However, we would decrement unconditionally, eventually underflowing.This PR removes the condition for incrementing. This preserves the behavior of being able to accept
Incomings after callingEndpoint::close. This is somewhat unlike what the iroh API expects (because in iroh you can't do anything with the endpoint afterEndpoint::close), but noq's API is just different.This also adds a regression test that fails without the fix. That needs a
PanicPropagatingRuntimeimplementation and some test helpers to allow for configuring that though...Breaking Changes
None
Notes & open questions
Disclosure: Heavily assisted with GLM 5.2.
Change checklist