feat(python): add QuicConfig transport configuration - #3991
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3991 +/- ##
============================================
+ Coverage 84.91% 84.92% +0.01%
Complexity 1405 1405
============================================
Files 1224 1224
Lines 179301 179572 +271
Branches 145615 145614 -1
============================================
+ Hits 152250 152501 +251
+ Misses 23024 23020 -4
- Partials 4027 4051 +24
🚀 New features to boost your workflow:
|
| /// Converts a Python timedelta to milliseconds, for fields the Rust SDK | ||
| /// stores as a raw millisecond count rather than an `IggyDuration` (e.g. | ||
| /// QUIC's `keep_alive_interval`/`max_idle_timeout`). | ||
| pub fn py_delta_to_millis(delta: &Py<PyDelta>) -> PyResult<u64> { |
There was a problem hiding this comment.
as_millis() truncates, and 0 is a magic value in configure():
QuicConfig(keep_alive_interval=timedelta(microseconds=500)) # -> 0 -> keep-alive off
QuicConfig(max_idle_timeout=timedelta(microseconds=500)) # -> 0 -> quinn's 30 s defaultPlease raise ValueError when a non-zero duration rounds down to 0 ms.
| /// receive_window: Receive window size in bytes. Defaults to 100,000. | ||
| /// keep_alive_interval: Interval between QUIC keep-alive pings, or a zero | ||
| /// duration to disable them. Defaults to 5 seconds. | ||
| /// max_idle_timeout: How long the connection tolerates silence before it is |
There was a problem hiding this comment.
The max_idle_timeout docs say a zero duration means "no limit". It actually means "quinn's 30 s default", because configure() skips the setter entirely.
| } | ||
| if let Some(max_concurrent_bidi_streams) = max_concurrent_bidi_streams { | ||
| inner.max_concurrent_bidi_streams = | ||
| u64_param(max_concurrent_bidi_streams, "max_concurrent_bidi_streams")?; |
There was a problem hiding this comment.
max_concurrent_bidi_streams and receive_window are validated as u64, but they go through VarInt::try_from (max 2^62 - 1).
| u64_param(datagram_send_buffer_size, "datagram_send_buffer_size")?; | ||
| } | ||
| if let Some(initial_mtu) = initial_mtu { | ||
| inner.initial_mtu = u16_param(initial_mtu, "initial_mtu")?; |
There was a problem hiding this comment.
quinn clamps this: TransportConfig::initial_mtu does value.max(1200). So QuicConfig(initial_mtu=500).initial_mtu reads back 500 while the connection runs at 1200. Please reject anything below 1200.
| auto_login=AutoLogin.username_password("iggy", "iggy"), | ||
| ) | ||
| ) | ||
| await client.connect() |
There was a problem hiding this comment.
This and test_without_auto_login_a_privileged_call_is_unauthenticated use the default unlimited reconnection, so a missing QUIC listener hangs until the CI timeout instead of failing.
test_wrong_auto_login_credentials_fail below already passes QuicReconnectionConfig(enabled=False). Same here?
Reject durations that round down to 0ms, fix the max_idle_timeout docstring, validate max_concurrent_bidi_streams/receive_window against VarInt::MAX, reject initial_mtu below quinn's 1200 floor, and disable reconnection in the two auto-login tests that could otherwise hang.
52d0f86 to
b56981f
Compare
|
Thanks for the review @ethanlin01x Could you check now i've addressed the following
|
| asyncio.run(main()) | ||
| ``` | ||
|
|
||
| `IggyClient.quic(...)` takes a `QuicConfig` the same way, built from `IggyClient.quic()`'s own | ||
| config type rather than passed to `IggyClient(...)`: | ||
|
|
||
| ```python | ||
| import asyncio | ||
| from datetime import timedelta | ||
|
|
||
| from apache_iggy import AutoLogin, IggyClient, QuicConfig, QuicReconnectionConfig | ||
|
|
||
|
|
||
| async def main(): | ||
| client = IggyClient.quic( | ||
| QuicConfig( | ||
| server_address="127.0.0.1:8080", | ||
| server_name="localhost", | ||
| auto_login=AutoLogin.username_password("iggy", "iggy"), | ||
| reconnection=QuicReconnectionConfig( | ||
| enabled=True, | ||
| max_retries=10, | ||
| interval=timedelta(seconds=2), | ||
| reestablish_after=timedelta(seconds=30), | ||
| ), | ||
| heartbeat_interval=timedelta(seconds=5), | ||
| # validate_certificate=True, | ||
| ) | ||
| ) | ||
| await client.connect() | ||
|
|
||
|
|
There was a problem hiding this comment.
i dont think we need that much code in main README.md for python, remove it
github-actions
Bot
added
S-waiting-on-author
slbotbm
left a comment
slbotbm
left a comment
There was a problem hiding this comment.
Let's fold the config example code into existing examples as comments, and expose IggyClient (... | QuicConfig | ... ) instead of IggyClient.quic method.
4 participants
Which issue does this PR address?
Relates to #2835.
Rationale
Python's QUIC transport was only reachable through the untested from_connection_string() path, with no config object like TCP got in #3776.
What changed?
Added QuicConfig/QuicReconnectionConfig, accepted by IggyClient.quic(...), mirroring the TcpConfig pattern. Also fixed a bug where from_connection_string() failed on iggy+quic:// URLs because quinn::Endpoint::client needs an active Tokio runtime context.
Local Execution