EarthChen commented on PR #16028: URL: https://github.com/apache/dubbo/pull/16028#issuecomment-3772237234
# Why change from `channel.isWritable()` to byte counting? ### Background The original implementation used Netty's `channel.isWritable()` to determine `isReady()` state, which had the following issues: 1. **Timing issue**: When a stream is just created, `channel.isWritable()` may return `false`, causing the initial `onReadyHandler` to fire with `isReady()=false`, preventing the client from sending data. 2. **Unreliable events**: The `channelWritabilityChanged` event only fires when Netty channel's high/low watermarks are crossed, which is unrelated to HTTP/2 flow control windows, potentially causing missed `onReady` callbacks. 3. **gRPC incompatibility**: gRPC uses byte counting approach. This difference may cause interoperability issues with gRPC clients/servers. ### New Implementation Uses a byte counting approach consistent with gRPC: - Track pending bytes with `numSentBytesQueued` - Threshold `ON_READY_THRESHOLD = 32KB` (same as gRPC) - Initial `numSentBytesQueued=0` ensures `isReady()=true` - Increment count when sending, decrement after send completes - Trigger `onReady` when count drops from `>=32KB` to `<32KB` ### Advantages of New Implementation 1. **Deterministic**: Initial state `numSentBytesQueued=0`, `isReady()` is guaranteed to be `true`, avoiding timing issues 2. **Reliable**: `onReady` triggering is fully controlled by byte counting, not dependent on external events, no missed callbacks 3. **Precise Control**: Based on actual bytes sent, not channel state, providing more accurate flow control 4. **Thread-safe**: Uses `AtomicLong` for atomic operations, supports high-concurrency scenarios 5. **Predictable**: Fixed threshold at 32KB, predictable behavior, easier to debug and test ### Limitations and Considerations 1. **Memory overhead**: Each stream maintains an `AtomicLong` counter, but the overhead is minimal (8 bytes) 2. **Network-unaware**: Byte counting only tracks send queue size, not actual network congestion. In extreme cases (e.g., network disconnected), may continue allowing sends until buffer is full 3. **Future callback dependency**: `onSentBytes` relies on write future callbacks. If Netty's future handling is delayed, it may affect `isReady()` accuracy -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
