Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚡️ Deallocate operations and requests for cancellation #2252

Merged
merged 19 commits into from
Jul 3, 2024

Conversation

AlexV525
Copy link
Member

@AlexV525 AlexV525 commented Jun 19, 2024

Resolve #2111

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 main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

@AlexV525
Copy link
Member Author

Apparently badssl.com is unstable these days. We might also explore deploying it locally in the future.

@AlexV525 AlexV525 marked this pull request as ready for review June 19, 2024 06:15
@AlexV525 AlexV525 requested a review from a team as a code owner June 19, 2024 06:15
@AlexV525 AlexV525 changed the title ⚡️ Use pool to deallocate operations and requests ⚡️ Deallocate operations and requests for cancellation Jun 20, 2024
@CaiJingLong
Copy link
Contributor

    test('cancels multiple requests', () async {
      final client = MockHttpClient();
      CancelToken? token = CancelToken();
      final tokenRef = WeakReference(token);
      const reason = 'cancel';
      final dio = Dio()
        ..httpClientAdapter = IOHttpClientAdapter(
          createHttpClient: () => client,
        );

      final requests = <MockHttpClientRequest>[];
      when(client.openUrl(any, any)).thenAnswer((_) async {
        final request = MockHttpClientRequest();
        requests.add(request);
        when(request.close()).thenAnswer((_) async {
          final response = MockHttpClientResponse();
          when(response.headers).thenReturn(MockHttpHeaders());
          when(response.statusCode).thenReturn(200);
          await Future.delayed(const Duration(milliseconds: 200));
          return response;
        });
        return request;
      });

      final futures = [
        dio.get('https://pub.dev', cancelToken: token),
        dio.get('https://pub.dev', cancelToken: token),
      ];

      for (final future in futures) {
        expectLater(
          future,
          throwsDioException(
            DioExceptionType.cancel,
            matcher: isA<DioException>().having(
              (e) => e.error,
              'error',
              reason,
            ),
          ),
        );
      }

      await Future.delayed(const Duration(milliseconds: 100));
      token.cancel(reason);

      expect(requests, hasLength(2));

      try {
        await Future.wait(futures);
      } catch (_) {
        // ignore, just waiting here till all futures are completed.
      }

      for (final request in requests) {
        verify(request.abort()).called(1);
      }

      token = null;

      Object? obj = Object();
      final objRef = WeakReference(obj);
      print(tokenRef.target);
      print(objRef.target);

      obj = null;

      List.generate(
        1 * 1024 * 1024,
        (index) => Object(),
      );

      print(tokenRef.target);
      print(objRef.target);

      expect(objRef.target, isNull);
      expect(tokenRef.target, isNull);
    });

I rewrite the method of cancel_token_test.dart.

I got next log:

Instance of 'CancelToken'
Instance of 'Object'
Instance of 'CancelToken'
null
Expected: null
  Actual: <Instance of 'CancelToken'>

package:matcher                    expect
test/cancel_token_test.dart 109:7  main.<fn>.<fn>

So, I think the cancel token isn't be deallocate in GC.

@CaiJingLong
Copy link
Contributor

BTW, maybe 1*1024*1024 is not big enough, you can use bigger.

@AlexV525 AlexV525 force-pushed the fix/cancel-token-memory-issue branch 3 times, most recently from e31512b to b2b5490 Compare June 27, 2024 14:35
@AlexV525 AlexV525 force-pushed the fix/cancel-token-memory-issue branch from b2b5490 to 74fd79f Compare June 27, 2024 14:38
@AlexV525
Copy link
Member Author

@kuhnroyal I've also configured the Melos script not to produce coverage outputs when running with the minimum SDK so the min test will not fail because of [Sentinel kind: Expired, valueAsString: <expired>] from getIsolateGroup(). That issue has been fixed by Dart 2.19.0.

@kuhnroyal
Copy link
Member

Do you have experience with https://pub.dev/packages/leak_tracker?

@AlexV525
Copy link
Member Author

AlexV525 commented Jul 1, 2024

Do you have experience with https://pub.dev/packages/leak_tracker?

I glanced at those packages, which seemed to not fit the current case and require a significant amount of work. Also not sure if they are compatible with older Dart. Spoken with @CaiJingLong and a stable way to test GC (also in Dart's tests) is generating big objects so here we have 1024 * 1024. This is what I've got so far.

@CaiJingLong
Copy link
Contributor

CaiJingLong commented Jul 2, 2024

There is a method in dart's testcase:

https://github.com/dart-lang/sdk/blob/59add4f01ef4741e10f64db9c2c8655cfe738ccb/tests/corelib/finalizer_test.dart#L86-L101

This method may be more in line with the rules of dart memory allocation.

void produceGarbage() {
  const approximateWordSize = 4;


  List<dynamic> sink = [];
  for (int i = 0; i < 500; ++i) {
    final filler = i % 2 == 0 ? 1 : sink;
    if (i % 250 == 1) {
      // 2 x 25 MB in old space.
      sink = List.filled(25 * 1024 * 1024 ~/ approximateWordSize, filler);
    } else {
      // 498 x 50 KB in new space
      sink = List.filled(50 * 1024 ~/ approximateWordSize, filler);
    }
  }
  print(sink.hashCode); // Ensure there's real use of the allocation.
}

@AlexV525
Copy link
Member Author

AlexV525 commented Jul 2, 2024

This method may be more in line with the rules of dart memory allocation.

Cool, now we can run that test on the minimal SDK requirement too.

Copy link
Contributor

github-actions bot commented Jul 2, 2024

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
dio/lib/src/adapters/io_adapter.dart 🟢 88.89% 🟢 88.75% 🔴 -0.14%
dio/lib/src/dio_mixin.dart 🟢 93.86% 🟢 93.99% 🟢 0.13%
plugins/http2_adapter/lib/src/http2_adapter.dart 🟠 71.72% 🟠 71.92% 🟢 0.2%
Overall Coverage 🟢 82.35% 🟢 82.44% 🟢 0.09%

Minimum allowed coverage is 0%, this run produced 82.44%

@AlexV525 AlexV525 merged commit 872429c into main Jul 3, 2024
3 checks passed
@AlexV525 AlexV525 deleted the fix/cancel-token-memory-issue branch July 3, 2024 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory issue when sharing CancelToken between posts for IO implementation
3 participants