feat: Add timeout handling for ConversionLayerAdapter #2456
Conversation
koji-1009
force-pushed
the
feat/timeout_native_adapter
branch
from
cf0f7d6 to
fdcda84
Compare
koji-1009
force-pushed
the
feat/timeout_native_adapter
branch
from
fdcda84 to
bc6c463
Compare
| // http package doesn't separate connect and receive phases, | ||
| // so we combine both timeouts for client.send() | ||
| final connectTimeout = options.connectTimeout ?? Duration.zero; | ||
| final receiveTimeout = options.receiveTimeout ?? Duration.zero; | ||
| final totalTimeout = connectTimeout + receiveTimeout; |
There was a problem hiding this comment.
Alternatively, it might be good to exclude connectionTimeout from support.
AlexV525
left a comment
AlexV525
left a comment
There was a problem hiding this comment.
Thanks for helping fix the behavior!
| Future<void>? cancelFuture, | ||
| ) async { | ||
| final request = await _fromOptionsAndStream( | ||
| final timeoutCompleter = Completer<void>(); |
There was a problem hiding this comment.
Since all onTimeout throws DioException, is the completer still valid?
There was a problem hiding this comment.
Yes, it's required. The completer is used to trigger the AbortableRequest's cancellation logic. The throw just handles reporting the error to the caller. We must signal the cancellation first, then throw.
AlexV525
left a comment
AlexV525
left a comment
There was a problem hiding this comment.
LGTM
The CI seems failing because of some request issues (not sure if it's related but it looks like not). Could you help to investigate?
|
Looking at https://github.com/cfug/dio/actions/workflows/tests.yml, it seems tests are failing in other PRs as well. I'll look into it. Thanks for maintaining dio and related packages. |
|
|
|
I created a pull request to fix the test case. |
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
|
Thank you! |
2 participants
fix #2457
Description
Implements timeout handling for
NativeAdapter. Previously, Dio's timeout options (sendTimeout,connectTimeout,receiveTimeout) were ignored when usingNativeAdapter.This PR adds proper timeout features in
ConversionLayerAdapter, following the same approach asio_adapter.dart.Note: Due to http package limitations,
connectTimeoutandreceiveTimeoutare combined for the response phase.New Pull Request Checklist
mainbranch to avoid conflicts (via merge from master or rebase)CHANGELOG.mdin the corresponding packageAdditional context and info (if any)