Clear test state flag #5689

kraenhansen · 2023-04-03T16:49:10Z

What, How & Why?

Instead of introducing a memory leak when running in production, this PR introduce a flag set from the tests to signal the SDK that it's allowed to store WeakRef instances into Sets without worrying about the memory leak, which would have happened if clearTestState was never called.

This also removes the IterableWeakRefs, as we no longer need something mimicking a WeakSet.

This also introduce the reset of SyncSession shared pointers and canceling unresolved async open tasks when clearing test state.

elle-j

Nice addition!

elle-j · 2023-04-03T17:25:02Z

integration-tests/tests/src/index.ts

+// We need this to call Realm.cleanTestState()
+flags.CLEAN_TEST_STATE = true;


Typo? CLEAR_ would align with the method name (rather than CLEAN_) 👍

Oh my. Thanks.

I've updated this and hopefully all other occurrences 🤞

elle-j · 2023-04-03T17:30:05Z

packages/realm/src/ProgressRealmPromise.ts

+      const promise = promiseRef.deref();
+      if (promise) {
+        promise.cancel();
+      }


Alternative

Suggested change

const promise = promiseRef.deref();

if (promise) {

promise.cancel();

}

promiseRef.deref()?.cancel();

elle-j · 2023-04-03T17:33:59Z

packages/realm/src/app-services/SyncSession.ts

@@ -262,6 +263,23 @@ const CONNECTION_LISTENERS = new Listeners<ConnectionNotificationCallback, Liste
 });

 export class SyncSession {
+  private static instances = new Set<binding.WeakRef<SyncSession>>();


We have the @internal on the instances on ProgressRealmPromise. Did we decide which one we wanted to converge on?

FWIW, there is less (but still not 0) benefit from keeping the private static members in the .d.ts vs the private instance members.

elle-j · 2023-04-03T17:35:37Z

packages/realm/src/app-services/SyncSession.ts

+      const session = sessionRef.deref();
+      if (session) {
+        session.resetInternal();
+      }


Alternative

Suggested change

const session = sessionRef.deref();

if (session) {

session.resetInternal();

}

sessionRef.deref()?.resetInternal();

elle-j · 2023-04-03T17:36:24Z

packages/realm/src/flags.ts

@@ -26,4 +26,9 @@ export const flags = {
   * Helps finding places where the app is depending on the now deprecated way of using the package.
   */
  THROW_ON_GLOBAL_REALM: false,
+  /**
+   * When enabled, calling `Realm.cleanTestState` will be callable.


Suggested change

* When enabled, calling `Realm.cleanTestState` will be callable.

* When enabled, calling `Realm.clearTestState` will be callable.

elle-j · 2023-04-03T17:37:15Z

integration-tests/tests/src/index.ts

@@ -22,6 +22,8 @@ import { flags } from "realm";

 // TODO: Refactor tests to disable this
 flags.ALLOW_VALUES_ARRAYS = true;
+// We need this to call Realm.cleanTestState()


Suggested change

// We need this to call Realm.cleanTestState()

// We need this to call Realm.clearTestState()

quashed commit of the following: commit 30c6c82 Author: Kræn Hansen <kraen.hansen@mongodb.com> Date: Mon Apr 3 19:33:08 2023 +0200 Fixed missing WeakRef wrapping commit a42c967 Author: Kræn Hansen <kraen.hansen@mongodb.com> Date: Mon Apr 3 19:27:35 2023 +0200 Canceling async tasks on clear test state commit 1022fba Author: Kræn Hansen <kraen.hansen@mongodb.com> Date: Mon Apr 3 19:21:46 2023 +0200 Capturing and cleaning WeakRef of SyncSessions commit 8befc51 Author: Kræn Hansen <kraen.hansen@mongodb.com> Date: Mon Apr 3 18:47:15 2023 +0200 Deleting the IterableWeakRefs commit 4645004 Author: Kræn Hansen <kraen.hansen@mongodb.com> Date: Mon Apr 3 18:45:55 2023 +0200 Introduce a CLEAN_TEST_STATE flag

RedBeard0531 · 2023-04-04T08:18:21Z

integration-tests/tests/src/index.ts

-// We need this to call Realm.cleanTestState()
-flags.CLEAN_TEST_STATE = true;
+// We need this to call Realm.clearTestState()
+flags.CLEAR_TEST_STATE = true;


Suggested change

flags.CLEAR_TEST_STATE = true;

flags.ENABLE_CLEAR_TEST_STATE = true;

Or ALLOW. The point is that setting the flag doesn't by itself clear the test state, it mearly enables (or allows) the test state to be cleared.

Also, reading this now, I think clearTestState() is slightly misnamed. Really it should be something like clearStateAfterTest() because it doesn't clear the test state, it tries to reset the general state of the world. But I don't suggest changing that right now.

I opted for ALLOW_CLEAR_TEST_STATE to align with the other existing flag.

And I agree, the function name is confusing. It's internal and can easily be renamed in another PR 👍

RedBeard0531 · 2023-04-04T08:22:28Z

packages/realm/src/ProgressRealmPromise.ts

+   * Cancels all unresolved `ProgressRealmPromise` instances.
+   * @internal
+   */
+  public static cancelAll() {


It seems weird to have a method in the middle of your instance variable declarations. Move this down?

I consider it "normal" to have all static members and methods before any instance methods and properties.

Fair enough. Could you at least separate the sections with a blank line then?

RedBeard0531 · 2023-04-04T08:25:44Z

packages/realm/src/app-services/SyncSession.ts

@@ -262,6 +263,23 @@ const CONNECTION_LISTENERS = new Listeners<ConnectionNotificationCallback, Liste
 });

 export class SyncSession {
+  private static instances = new Set<binding.WeakRef<SyncSession>>();


FWIW, there is less (but still not 0) benefit from keeping the private static members in the .d.ts vs the private instance members.

RedBeard0531 · 2023-04-04T08:26:34Z

packages/realm/src/flags.ts

@@ -26,4 +26,9 @@ export const flags = {
   * Helps finding places where the app is depending on the now deprecated way of using the package.
   */
  THROW_ON_GLOBAL_REALM: false,
+  /**
+   * When enabled, calling `Realm.clearTestState` will be callable.
+   * This is disabled by default, mainly because the data-structures needed to support this, introduce minor memory leaks and are not intended for production use.


Suggested change

* This is disabled by default, mainly because the data-structures needed to support this, introduce minor memory leaks and are not intended for production use.

* This is disabled by default, mainly because the data-structures needed to support this, introduce minor memory leaks if clearTestState() is not called regularly and are not intended for production use.

I added this.

RedBeard0531 · 2023-04-04T08:28:44Z

packages/realm/src/flags.ts

@@ -26,4 +26,9 @@ export const flags = {
   * Helps finding places where the app is depending on the now deprecated way of using the package.
   */
  THROW_ON_GLOBAL_REALM: false,
+  /**
+   * When enabled, calling `Realm.clearTestState` will be callable.


Suggested change

* When enabled, calling `Realm.clearTestState` will be callable.

* Enables calling internal, test-only functions like `Realm.clearTestState`.

better to use active rather than passive voice where possible.

the effects of this flag are already not limited to that function, so we might as well not lie.

I added this.

* main: (41 commits) allow useQuery to filter or sort a collection by using a callback (#5513) Fix `realm.create` types and 'requiredProperties' on Realm.Object constructor (#5697) Fix return type of `App.allUsers` (#5708) Removed renamed file Remove console.log Fix README instructions and bump version Bump template versions Update Templates (#5702) Moved "submodules" wireit task to "postinstall" Set up typedoc for realm-react, realm-web (#5709) Prepare for vNext (#5711) Prepare for 12.0.0-alpha.2 (#5707) Build iOS prebuilds in release by default (#5710) Use the event.sender as assignee when preparing release Updated prepare release workflow to print PR url in summary Fixing lint error fix the template app links (#5701) Expose `Sync` as named export (#5705) Enable all tests after bad rebase (#5595) Clear test state flag (#5689) ... # Conflicts: # packages/realm/src/Realm.ts

kraenhansen added 2 commits April 3, 2023 18:45

Introduce a CLEAN_TEST_STATE flag

4645004

Deleting the IterableWeakRefs

8befc51

kraenhansen marked this pull request as ready for review April 3, 2023 16:49

cla-bot bot added the cla: yes label Apr 3, 2023

kraenhansen requested review from RedBeard0531 and elle-j April 3, 2023 16:49

github-actions bot assigned kraenhansen Apr 3, 2023

kraenhansen mentioned this pull request Apr 3, 2023

Keep an IterableWeakRefs of SyncSessions to reset when clearing test state #5651

Closed

kraenhansen added 2 commits April 3, 2023 19:21

Capturing and cleaning WeakRef of SyncSessions

1022fba

Canceling async tasks on clear test state

a42c967

kraenhansen force-pushed the kh/clean-test-state-mode branch from 49124e2 to a42c967 Compare April 3, 2023 17:27

kraenhansen mentioned this pull request Apr 3, 2023

Cancel async tasks on clear test state #5667

Closed

kraenhansen changed the title ~~Clean test state mode~~ Clean test state flag Apr 3, 2023

Fixed missing WeakRef wrapping

30c6c82

elle-j approved these changes Apr 3, 2023

View reviewed changes

kraenhansen added 2 commits April 4, 2023 09:38

clean → clear

55f0403

Addressing feedback

8862c39

kraenhansen changed the title ~~Clean test state flag~~ Clear test state flag Apr 4, 2023

Fixed @realm/react tests

b0f48fa

RedBeard0531 approved these changes Apr 4, 2023

View reviewed changes

kraenhansen added 2 commits April 5, 2023 17:35

Rename "CLEAR_TEST_STATE" to "ALLOW_CLEAR_TEST_STATE"

18a5cfb

Updated documentation.

9a07009

kraenhansen merged commit 45dd275 into main Apr 5, 2023

kraenhansen deleted the kh/clean-test-state-mode branch April 5, 2023 15:35

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear test state flag #5689

Clear test state flag #5689

kraenhansen commented Apr 3, 2023 •

edited

Loading

elle-j left a comment

elle-j Apr 3, 2023

kraenhansen Apr 4, 2023

kraenhansen Apr 4, 2023

elle-j Apr 3, 2023

elle-j Apr 3, 2023

RedBeard0531 Apr 4, 2023

elle-j Apr 3, 2023

elle-j Apr 3, 2023

elle-j Apr 3, 2023

RedBeard0531 Apr 4, 2023

kraenhansen Apr 5, 2023 •

edited

Loading

RedBeard0531 Apr 4, 2023

kraenhansen Apr 5, 2023

RedBeard0531 Apr 5, 2023

RedBeard0531 Apr 4, 2023

RedBeard0531 Apr 4, 2023

kraenhansen Apr 5, 2023

RedBeard0531 Apr 4, 2023

kraenhansen Apr 5, 2023

		// We need this to call Realm.cleanTestState()
		flags.CLEAN_TEST_STATE = true;

	* When enabled, calling `Realm.cleanTestState` will be callable.
	* When enabled, calling `Realm.clearTestState` will be callable.

	// We need this to call Realm.cleanTestState()
	// We need this to call Realm.clearTestState()

	flags.CLEAR_TEST_STATE = true;
	flags.ENABLE_CLEAR_TEST_STATE = true;

	* This is disabled by default, mainly because the data-structures needed to support this, introduce minor memory leaks and are not intended for production use.
	* This is disabled by default, mainly because the data-structures needed to support this, introduce minor memory leaks if clearTestState() is not called regularly and are not intended for production use.

	* When enabled, calling `Realm.clearTestState` will be callable.
	* Enables calling internal, test-only functions like `Realm.clearTestState`.

Clear test state flag #5689

Clear test state flag #5689

Conversation

kraenhansen commented Apr 3, 2023 • edited Loading

What, How & Why?

elle-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen commented Apr 3, 2023 •

edited

Loading

kraenhansen Apr 5, 2023 •

edited

Loading