Implement Connection Pool API #4832
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4832 +/- ##
==========================================
- Coverage 87.49% 86.24% -1.26%
==========================================
Files 56 57 +1
Lines 17726 17956 +230
==========================================
- Hits 15510 15486 -24
- Misses 2216 2470 +254 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
anrossi
force-pushed
the
anrossi/connection-pool-api
branch
from
54be6ca to
47753f0
Compare
anrossi
force-pushed
the
anrossi/connection-pool-api
branch
from
d22ce49 to
acd61c6
Compare
src/core/connection_pool.c
Outdated
| Config->CibirIds ? Config->CibirIds[i] : NULL, | ||
| &Connections[i]); | ||
| if (QUIC_FAILED(Status)) { | ||
| continue; |
There was a problem hiding this comment.
If QuicConnAlloc() is successful inside QuicConnPoolTryCreateConnection, then should you free that memory and close connection here?
There was a problem hiding this comment.
I think if the QuicConnAlloc is successful, but an error is returned by a later operation inside TryCreateConnection, then TryCreateConnection should clean up the connection.
There was a problem hiding this comment.
I think QuicConnPoolTryCreateConnection() is not cleaning up the connection, in cases like QuicConnParamSet() fails then "goto Error" is hit; pls check.
| } QUIC_CONNECTION_POOL_FLAGS; | ||
|
|
||
| typedef struct QUIC_CONNECTION_POOL_CONFIG { | ||
| _In_ HQUIC Registration; |
There was a problem hiding this comment.
Need to fix up all the SAL annotations here to match the header.
| _At_buffer_(_Curr_, _Iter_, NumberOfConnections, _Field_size_(CibirIdLength)) | ||
| _Field_size_opt_(NumberOfConnections) | ||
| uint8_t** CibirIds; // Optional | ||
| _In_ uint8_t CibirIdLength; // Optional |
There was a problem hiding this comment.
To be clear, you must set this to zero if not using CIBIR, so it's not strictly optional. Please make sure to update msquic.h too if necessary.
|
|
||
| `Configuration` | ||
|
|
||
| An opened Configuration that provides the settings used to create each connection in the pool. |
There was a problem hiding this comment.
| An opened Configuration that provides the settings used to create each connection in the pool. | |
| The valid handle to an open and loaded configuration object that is used to create each connection in the pool. |
This is more inline with the wording we use in ConnectionStart.md
There was a problem hiding this comment.
I recommend you do pretty much the same for the rest of the parameters, so we can have consistent docs.
|
|
||
| `Registration` | ||
|
|
||
| An opened Registration for creating the connection in. |
There was a problem hiding this comment.
| An opened Registration for creating the connection in. | |
| The valid handle to an open registration object. |
Copied from ConnectionOpen.md
|
|
||
| `CibirIdLength` | ||
|
|
||
| The number of bytes in each CIBIR ID. Not allowed to be zero if `CibirIds` is non-NULL. |
There was a problem hiding this comment.
I'd say we should simply ignore CibirIds if this is zero. No need to be over restrictive.
There was a problem hiding this comment.
that brings up an interesting question about our API: do we test if the pointer is NULL, or do we look at the length field?
| ) | ||
| { | ||
| CXPLAT_SOCKET* Socket = NULL; | ||
| CXPLAT_UDP_CONFIG UdpConfig = { .RemoteAddress = RemoteAddress, }; |
There was a problem hiding this comment.
| CXPLAT_UDP_CONFIG UdpConfig = { .RemoteAddress = RemoteAddress, }; | |
| CXPLAT_UDP_CONFIG UdpConfig = { .RemoteAddress = RemoteAddress, 0 }; |
Let's be a bit more specific here
| goto Error; | ||
| } | ||
| (*Connection)->ClientCallbackHandler = Handler; | ||
| if (Context != NULL) { |
There was a problem hiding this comment.
No real need to check if NULL here.
|
|
||
|
|
| "[ lib] ERROR, %u, %s.", | ||
| Status, | ||
| "Connection Pool Parameter"); | ||
| return Status; |
There was a problem hiding this comment.
You're not logging API exit by doing a return now.
| // The connection either owns the ServerNameCopy, or it was freed. | ||
| // | ||
| ServerNameCopy = NULL; | ||
| if (Status == QUIC_STATUS_OUT_OF_MEMORY) { |
There was a problem hiding this comment.
Personally, I wouldn't special case out of memory errors. For one, they may be transient, and two there might be many errors you could special case, but it unnecessarily complicates the logic for such rare edge cases.
Description
Provide an API for pooling connections across multiple RSS cores
Testing
Manual testing
Documentation
Documentation will be updated with the new API