-
Notifications
You must be signed in to change notification settings - Fork 10
Improve DioError
s
#16
Changes from 2 commits
3e587ca
28f1fd0
275a984
5aba8b6
7727460
9c6af33
1f4ff87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,19 +34,19 @@ class DefaultHttpClientAdapter implements HttpClientAdapter { | |
var _httpClient = _configHttpClient(cancelFuture, options.connectTimeout); | ||
var reqFuture = _httpClient.openUrl(options.method, options.uri); | ||
|
||
void _throwConnectingTimeout() { | ||
throw DioError( | ||
requestOptions: options, | ||
error: 'Connecting timed out [${options.connectTimeout}ms]', | ||
type: DioErrorType.connectTimeout, | ||
); | ||
} | ||
|
||
late HttpClientRequest request; | ||
try { | ||
final connectionTimeout = options.connectTimeout; | ||
if (connectionTimeout != null) { | ||
request = await reqFuture.timeout(connectionTimeout); | ||
request = await reqFuture.timeout( | ||
connectionTimeout, | ||
onTimeout: () { | ||
throw DioError.connectionTimeout( | ||
requestOptions: options, | ||
timeout: connectionTimeout, | ||
); | ||
}, | ||
); | ||
} else { | ||
request = await reqFuture; | ||
} | ||
|
@@ -55,13 +55,18 @@ class DefaultHttpClientAdapter implements HttpClientAdapter { | |
options.headers.forEach((k, v) { | ||
if (v != null) request.headers.set(k, '$v'); | ||
}); | ||
} on SocketException catch (e) { | ||
if (e.message.contains('timed out')) { | ||
_throwConnectingTimeout(); | ||
} on SocketException catch (e, stackTrace) { | ||
if (!e.message.contains('timed out')) { | ||
rethrow; | ||
} | ||
rethrow; | ||
} on TimeoutException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it necessary to remove this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the exceptions are now thrown in the
See https://api.dart.dev/stable/2.18.4/dart-async/Future/timeout.html |
||
_throwConnectingTimeout(); | ||
throw DioError.connectionTimeout( | ||
requestOptions: options, | ||
timeout: options.connectTimeout ?? | ||
_httpClient.connectionTimeout ?? | ||
Duration.zero, | ||
error: e, | ||
stackTrace: stackTrace, | ||
); | ||
} | ||
|
||
request.followRedirects = options.followRedirects; | ||
|
@@ -72,37 +77,38 @@ class DefaultHttpClientAdapter implements HttpClientAdapter { | |
var future = request.addStream(requestStream); | ||
final sendTimeout = options.sendTimeout; | ||
if (sendTimeout != null) { | ||
future = future.timeout(sendTimeout); | ||
} | ||
try { | ||
await future; | ||
} on TimeoutException { | ||
request.abort(); | ||
throw DioError( | ||
requestOptions: options, | ||
error: 'Sending timeout[${options.sendTimeout}ms]', | ||
type: DioErrorType.sendTimeout, | ||
future = future.timeout( | ||
sendTimeout, | ||
onTimeout: () { | ||
request.abort(); | ||
throw DioError.sendTimeout( | ||
timeout: sendTimeout, | ||
requestOptions: options, | ||
); | ||
}, | ||
); | ||
} | ||
|
||
await future; | ||
} | ||
|
||
final stopwatch = Stopwatch()..start(); | ||
var future = request.close(); | ||
final receiveTimeout = options.receiveTimeout; | ||
if (receiveTimeout != null) { | ||
future = future.timeout(receiveTimeout); | ||
} | ||
late HttpClientResponse responseStream; | ||
try { | ||
responseStream = await future; | ||
} on TimeoutException { | ||
throw DioError( | ||
requestOptions: options, | ||
error: 'Receiving data timeout[${options.receiveTimeout}]', | ||
type: DioErrorType.receiveTimeout, | ||
future = future.timeout( | ||
receiveTimeout, | ||
onTimeout: () { | ||
throw DioError.receiveTimeout( | ||
timeout: receiveTimeout, | ||
requestOptions: options, | ||
); | ||
}, | ||
); | ||
} | ||
|
||
final responseStream = await future; | ||
|
||
var stream = | ||
responseStream.transform<Uint8List>(StreamTransformer.fromHandlers( | ||
handleData: (data, sink) { | ||
|
@@ -111,10 +117,9 @@ class DefaultHttpClientAdapter implements HttpClientAdapter { | |
final receiveTimeout = options.receiveTimeout; | ||
if (receiveTimeout != null && duration > receiveTimeout) { | ||
sink.addError( | ||
DioError( | ||
DioError.receiveTimeout( | ||
timeout: receiveTimeout, | ||
requestOptions: options, | ||
error: 'Receiving data timeout[${options.receiveTimeout}]', | ||
type: DioErrorType.receiveTimeout, | ||
), | ||
); | ||
responseStream.detachSocket().then((socket) => socket.destroy()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,35 +2,109 @@ import 'options.dart'; | |
import 'response.dart'; | ||
|
||
enum DioErrorType { | ||
/// It occurs when url is opened timeout. | ||
connectTimeout, | ||
/// Caused by a connection timeout. | ||
connectionTimeout, | ||
|
||
/// It occurs when url is sent timeout. | ||
sendTimeout, | ||
|
||
///It occurs when receiving timeout. | ||
receiveTimeout, | ||
|
||
/// When the server response, but with a incorrect status, such as 404, 503... | ||
response, | ||
/// The [DioError] was caused by an incorrect status code as configured by | ||
/// [ValidateStatus]. | ||
badResponse, | ||
|
||
/// When the request is cancelled, dio will throw a error with this type. | ||
cancel, | ||
|
||
/// Caused for example by a `xhr.onError` or SocketExceptions. | ||
connectionError, | ||
|
||
/// Default error type, Some other Error. In this case, you can | ||
/// use the DioError.error if it is not null. | ||
other, | ||
unknown, | ||
} | ||
|
||
extension _DioErrorTypeExtension on DioErrorType { | ||
String toPrettyDescription() { | ||
switch (this) { | ||
case DioErrorType.connectionTimeout: | ||
return 'connection timeout'; | ||
case DioErrorType.sendTimeout: | ||
return 'send timeout'; | ||
case DioErrorType.receiveTimeout: | ||
return 'receive timeout'; | ||
case DioErrorType.badResponse: | ||
return 'bad response'; | ||
case DioErrorType.cancel: | ||
return 'request cancelled'; | ||
case DioErrorType.connectionError: | ||
return 'connection error'; | ||
case DioErrorType.unknown: | ||
return 'unknown'; | ||
} | ||
} | ||
} | ||
|
||
/// DioError describes the error info when request failed. | ||
/// DioError describes the exception info when a request failed. | ||
class DioError implements Exception { | ||
/// Prefer using one of the other constructors. | ||
/// They're most likely better fitting. | ||
DioError({ | ||
required this.requestOptions, | ||
this.response, | ||
this.type = DioErrorType.other, | ||
this.type = DioErrorType.unknown, | ||
this.error, | ||
this.stackTrace, | ||
this.message, | ||
}); | ||
|
||
DioError.badResponse({ | ||
required int statusCode, | ||
required this.requestOptions, | ||
required this.response, | ||
}) : type = DioErrorType.badResponse, | ||
message = 'The request returned an ' | ||
'invalid status code of $statusCode'; | ||
ueman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
DioError.connectionTimeout({ | ||
required Duration timeout, | ||
required this.requestOptions, | ||
this.error, | ||
this.stackTrace, | ||
}) : type = DioErrorType.connectionTimeout, | ||
message = 'The request connection took ' | ||
'longer than $timeout. It was aborted.'; | ||
|
||
DioError.sendTimeout({ | ||
required Duration timeout, | ||
required this.requestOptions, | ||
}) : type = DioErrorType.sendTimeout, | ||
message = 'The request took ' | ||
'longer than $timeout to send data. It was aborted.'; | ||
|
||
DioError.receiveTimeout({ | ||
required Duration timeout, | ||
required this.requestOptions, | ||
}) : type = DioErrorType.receiveTimeout, | ||
message = 'The request took ' | ||
'longer than $timeout to receive data. It was aborted.'; | ||
|
||
DioError.requestCancelled({ | ||
required this.requestOptions, | ||
required Object? reason, | ||
this.stackTrace, | ||
}) : type = DioErrorType.cancel, | ||
message = 'The request was cancelled.', | ||
error = reason; | ||
|
||
DioError.connectionError({ | ||
required this.requestOptions, | ||
required String reason, | ||
}) : type = DioErrorType.connectionError, | ||
message = 'The connection errored: $reason'; | ||
|
||
/// Request info. | ||
RequestOptions requestOptions; | ||
|
||
|
@@ -42,24 +116,26 @@ class DioError implements Exception { | |
|
||
/// The original error/exception object; It's usually not null when `type` | ||
/// is DioErrorType.other | ||
dynamic error; | ||
|
||
StackTrace? _stackTrace; | ||
Object? error; | ||
|
||
set stackTrace(StackTrace? stack) => _stackTrace = stack; | ||
/// The stacktrace of the original error/exception object; | ||
/// It's usually not null when `type` is DioErrorType.other | ||
StackTrace? stackTrace; | ||
|
||
StackTrace? get stackTrace => _stackTrace; | ||
|
||
String get message => (error?.toString() ?? ''); | ||
String? message; | ||
|
||
@override | ||
String toString() { | ||
var msg = 'DioError [$type]: $message'; | ||
if (error is Error) { | ||
msg += '\n${(error as Error).stackTrace}'; | ||
var msg = 'DioError [${type.toPrettyDescription()}]: $message'; | ||
final error = this.error; | ||
if (error != null) { | ||
msg += '\nError: $error'; | ||
} | ||
if (error is Error && error.stackTrace != stackTrace) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to hide stack traces from when describing a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you prefer it? I would prefer to get rid of the wrapping of exceptions, which is the cause of it. But as long as the wrapping of exceptions is done, the inner stacktrace might be helpful for debugging, so it should be included in the message. Otherwise, there's no way to get that stacktrace when using error monitoring tools. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it holds the stack trace. The stack trace can be accessed as a field of |
||
msg += '\nInner error stacktrace:\n${error.stackTrace}'; | ||
} | ||
if (_stackTrace != null) { | ||
msg += '\nSource stack:\n$stackTrace'; | ||
if (stackTrace != null) { | ||
msg += '\nInner stacktrace:\n$stackTrace'; | ||
} | ||
return msg; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking around the code again, should we return the closure here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might figure out a proper test for this, but no need to rush.