Improve `DioError`s #1405

ueman · 2022-01-26T15:09:19Z

NOTE: This goes against the develop-5.0 branch, since it's a breaking change.

New Pull Request Checklist

I have read the Documentation
I have searched for a similar pull request in the project and found none
I have updated this branch with the latest develop to avoid conflicts (via merge from master or rebase)
I have added the required tests to prove the fix/feature I am adding
I have updated the documentation (if necessary)
I have run the tests and they pass

Pull Request Description

This significantly improves the inner stacktraces of the DioError.

This reverts commit 0107079. # Conflicts: # dio/CHANGELOG.md

jgoyvaerts · 2022-01-26T15:54:04Z

Good idea! Can you include an example of a stacktrace message before and after your PR so people can see the difference?

ueman · 2022-01-26T16:07:41Z

Good idea! Can you include an example of a stacktrace message before and after your PR so people can see the difference?

I'm still thinking about how to produce those cases, in order to test it. Do you have any ideas?

kuhnroyal · 2022-01-28T10:34:03Z

Just a reminder too myself: Consider raising to Dart 2.16 and using Error.rethrowWithStackTrace.

ueman · 2022-01-30T17:11:27Z

@kuhnroyal Do you mind sharing your test cases? Then I can go through them.

kuhnroyal · 2022-01-30T17:46:27Z

It's nothing special, I only tested:

404 response from a random URL (this has no inner error as the response is received)
Something like post<void>(url, data: ArgumentError()) - this throws a serialization error an has an inner error/stacktrace

fix: abort request when timeout occurs

…velop � Conflicts: � dio/CHANGELOG.md

Correct JSON MIME Type detection

…velop

AlexV525

Hi @ueman. The change looks valid overall, with only a few nit-pickings.

Also, there are a few things I'm considering:

Are you willing to contribute by letting us pick the PR to another community-maintained dio? (We'll announce the new repo later once we merge PRs as much as possible.)
It might be better to add some tests.
.DS_Store can be removed from the PR.

dio/lib/src/adapters/browser_adapter.dart

dio/lib/src/adapters/io_adapter.dart

AlexV525 · 2022-10-19T05:52:02Z

dio/lib/src/dio_error.dart

-  /// It occurs when url is opened timeout.
-  connectTimeout,
+  /// Caused by a connection timeout.
+  connectionTimeout,


The changes with DioErrorType should be considered as a breaking change, which means you'll need to update the migration guide?

Yes, this is a breaking change

I don't think I need to add anything to the migration guide. From a user's point of view, nothing changes. The user can still catch exceptions the same way as before. All breaking changes are done from within the package in this PR.

ueman · 2022-10-19T06:15:49Z

Hi @ueman. The change looks valid overall, with only a few nit-pickings.

Also, there are a few things I'm considering:

Are you willing to contribute by letting us pick the PR to another community-maintained dio? (We'll announce the new repo later once we merge PRs as much as possible.)

Sure.

It might be better to add some tests.

I agree, any specific suggestions for tests? Just so I don't miss anything, you might think is important.

.DS_Store can be removed from the PR.

Yes, good point

Co-authored-by: Alex Li <[email protected]>

AlexV525 · 2022-10-19T06:21:04Z

Sure.

Thanks!

I agree, any specific suggestions for tests? Just so I don't miss anything, you might think is important.

IMO tests should be covered by checking if requests throw corresponding exceptions in condition.

# Conflicts: # dio/CHANGELOG.md # dio/lib/src/adapters/io_adapter.dart # dio/lib/src/options.dart

AlexV525

Given that we've already merged a couple PRs in the fork, I would suggest that we can submit this PR to the fork directly.

AlexV525 · 2022-10-24T02:29:39Z

dio/lib/src/dio_mixin.dart

@@ -630,6 +636,11 @@ abstract class DioMixin implements Dio {
        }
      }

+      StackTrace? stackTrace;
+      if (maybeStackTrace is StackTrace?) {


maybeStackTrace will always be StackTrace. The condition can be maybeStackTrace != StackTrace.empty.

AlexV525 · 2022-10-24T02:31:16Z

dio/CHANGELOG.md


-# 4.0.5-beta1
+- Add option to instantiate a `HttpClientAdapter`, which is platform independent


Seems like we need to change the base branch to develop.

AlexV525 · 2022-10-24T02:31:34Z

dio/lib/src/adapters/browser_adapter.dart

-              StackTrace.current,
-            );
-            xhr.abort();
+          if (haveSent) {


This will be fix by another PR.

AlexV525 · 2022-10-24T02:31:50Z

dio/lib/src/dio_error.dart


  /// When the request is cancelled, dio will throw a error with this type.
-  cancel,
+  requestCancelled,


Users usually ignore canceled requests since they are meant to do it, which means they'll rely on the en. Maybe we can keep the naming at least for the least breaking.

AlexV525

I would now suggest submitting the PR to the fork directly. Also, the target branch should be main.

ueman · 2022-11-03T17:50:42Z

I would now suggest submitting the PR to the fork directly. Also, the target branch should be main.

I don't have the permissions to do that

AlexV525 · 2022-11-03T18:46:14Z

I don't have the permissions to do that

Sorry. It should be granted now.

AlexV525 · 2022-12-18T16:00:00Z

Hi everyone! We've made our hardfork repo public and published a new version of dio, named diox.
The new package contains the PR of the fix.
Please refer to https://pub.dev/packages/diox/versions/5.0.0-dev.1 to use the fork.
You can also see why we're working for a hardfork at cfug/diox#29 and #1607.

kuhnroyal · 2023-03-07T12:07:39Z

I am missing a lot of stacktraces in DioErrors after this change.
This was merged in the diox repository.

kuhnroyal · 2023-03-07T12:19:47Z

All the server response errors have no stackTrace at all. It is impossible to tell where the error originated.

[Improve nullability in DioMixin.assureResponse](8418c72) * there should never be a case where there are not `RequestOptions` available * this is technically a breaking change but we should instead mark this `@internal` in the future [Fix missing source stacktrace in DioError](4f4ffe4) * a `DioError` should always have a meaningful stacktrace which points to the actual invocation of `Dio.get/post/xxx` * `Dio` historically lost a lot of this source information due to a multitude of asynchronous calls * now the source stacktrace is consistently being set into the `RequestOptions` instance and can later be retrieved and used when a `DioError` is constructed * additionally a `DioError` may contain another cause which may itself contain a separate stackTrace with more detailed information ### New Pull Request Checklist - [x] I have read the [Documentation](https://pub.dev/documentation/dio/latest/) - [x] I have searched for a similar pull request in the [project](https://github.com/cfug/dio/pulls) and found none - [x] I have updated this branch with the latest `main` branch to avoid conflicts (via merge from master or rebase) - [x] I have added the required tests to prove the fix/feature I'm adding - [x] I have updated the documentation (if necessary) - [x] I have run the tests without failures - [x] I have updated the `CHANGELOG.md` in the corresponding package ### Additional context and info (if any) The existing behavior which was somewhat correct got lost in #1405 which was merged in the temporary diox repository. --------- Signed-off-by: Alex Li <[email protected]> Co-authored-by: Alex Li <[email protected]>

morlay and others added 4 commits January 20, 2022 10:26

Fix BrowserHttpClientAdapter canceled hangs

96e1ef0

Revert "It's safe for status code to be non-null"

95c004e

This reverts commit 0107079. # Conflicts: # dio/CHANGELOG.md

Merge pull request cfug#1404 from ueman/revert-non-null-status-code

cbf8c8f

improve inner stacktraces

283bb35

ueman marked this pull request as draft January 26, 2022 15:09

Ascenio and others added 3 commits January 26, 2022 23:53

docs: fix table of contents links

271eebd

Improve DioError

0e11a48

improve error

c61c8eb

ueman changed the title ~~Improve inner stacktraces of DioError~~ Improve DioErrors Jan 27, 2022

ueman mentioned this pull request Jan 28, 2022

Feature: Improve Dio exception reports getsentry/sentry-dart#718

Merged

5 tasks

ueman added 2 commits January 28, 2022 19:55

wip

4beed0b

wip

c17462e

ueman and others added 13 commits January 30, 2022 20:13

code improvement

1df2d46

fix tests

2637269

better code flow

1853a30

only print stacktrace if not duplicated

6588021

ci: Remove dev channel, now always points to beta

fbee370

Merge pull request cfug#1411 from kuhnroyal/feature/remove-dev-channel

0e6753c

Merge pull request cfug#1406 from Ascenio/patch-2

1053fb6

ci: Upload and collect test results for all sdks

39d6f83

Merge pull request cfug#1412 from kuhnroyal/ci/split-test-results

349ba31

Merge branch 'develop' into morley-deveop

4df73f6

Add changelog and update workflow

0a7b891

Merge pull request cfug#1389 from morlay/develop

b4bb6ff

ci: Run checkout in test reporter workflow

2872c10

wendux and others added 8 commits March 29, 2022 17:09

Merge pull request cfug#1440 from amondnet/abort-connection

2782c1a

fix: abort request when timeout occurs

update doc

a5d84a4

Merge branch 'develop' of https://github.com/flutterchina/dio into de…

7886329

…velop � Conflicts: � dio/CHANGELOG.md

Merge pull request cfug#1439 from lukaskirner/develop

545142c

Correct JSON MIME Type detection

update changelog

ab0c93e

Merge branch 'develop' of https://github.com/flutterchina/dio into de…

6ef257a

…velop

release 4.0.5

f56ce11

fix cfug#1452

ac78e61

ueman mentioned this pull request Aug 3, 2022

Interceptors now gets the StackTrace in the DioError objects #1503

Closed

6 tasks

AlexV525 reviewed Oct 19, 2022

View reviewed changes

ueman and others added 2 commits October 19, 2022 08:16

Update dio/lib/src/adapters/browser_adapter.dart

0e3e3f2

Co-authored-by: Alex Li <[email protected]>

Update dio/lib/src/adapters/io_adapter.dart

23c0c3f

ueman and others added 4 commits October 21, 2022 19:36

Merge branch 'develop' into improve-stacktraces

bb3c6e3

# Conflicts: # dio/CHANGELOG.md # dio/lib/src/adapters/io_adapter.dart # dio/lib/src/options.dart

Delete .DS_Store

0bb6422

don't force unwrap

92b5617

fix changelog

38c0b72

AlexV525 reviewed Oct 24, 2022

View reviewed changes

AlexV525 reviewed Nov 3, 2022

View reviewed changes

This was referenced Nov 3, 2022

Use Durations instead of ints for timeouts cfug/diox#12

Merged

Improve DioErrors cfug/diox#16

Merged

AlexV525 closed this Feb 13, 2023

kuhnroyal mentioned this pull request Mar 7, 2023

[dio] Fix missing source stacktrace in DioError #1722

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `DioError`s #1405

Improve `DioError`s #1405

ueman commented Jan 26, 2022 •

edited

Loading

jgoyvaerts commented Jan 26, 2022

ueman commented Jan 26, 2022

kuhnroyal commented Jan 28, 2022

ueman commented Jan 30, 2022

kuhnroyal commented Jan 30, 2022

AlexV525 left a comment

AlexV525 Oct 19, 2022

ueman Oct 19, 2022

ueman Oct 21, 2022

ueman commented Oct 19, 2022

AlexV525 commented Oct 19, 2022

AlexV525 left a comment

AlexV525 Oct 24, 2022

AlexV525 Oct 24, 2022

AlexV525 Oct 24, 2022

AlexV525 Oct 24, 2022

AlexV525 left a comment

ueman commented Nov 3, 2022

AlexV525 commented Nov 3, 2022

AlexV525 commented Dec 18, 2022

kuhnroyal commented Mar 7, 2023

kuhnroyal commented Mar 7, 2023


		# 4.0.5-beta1
		- Add option to instantiate a `HttpClientAdapter`, which is platform independent

Improve DioErrors #1405

Improve DioErrors #1405

Conversation

ueman commented Jan 26, 2022 • edited Loading

New Pull Request Checklist

Pull Request Description

jgoyvaerts commented Jan 26, 2022

ueman commented Jan 26, 2022

kuhnroyal commented Jan 28, 2022

ueman commented Jan 30, 2022

kuhnroyal commented Jan 30, 2022

AlexV525 left a comment

Choose a reason for hiding this comment

AlexV525 Oct 19, 2022

Choose a reason for hiding this comment

ueman Oct 19, 2022

Choose a reason for hiding this comment

ueman Oct 21, 2022

Choose a reason for hiding this comment

ueman commented Oct 19, 2022

AlexV525 commented Oct 19, 2022

AlexV525 left a comment

Choose a reason for hiding this comment

AlexV525 Oct 24, 2022

Choose a reason for hiding this comment

AlexV525 Oct 24, 2022

Choose a reason for hiding this comment

AlexV525 Oct 24, 2022

Choose a reason for hiding this comment

AlexV525 Oct 24, 2022

Choose a reason for hiding this comment

AlexV525 left a comment

Choose a reason for hiding this comment

ueman commented Nov 3, 2022

AlexV525 commented Nov 3, 2022

AlexV525 commented Dec 18, 2022

kuhnroyal commented Mar 7, 2023

kuhnroyal commented Mar 7, 2023

Improve `DioError`s #1405

Improve `DioError`s #1405

ueman commented Jan 26, 2022 •

edited

Loading