Hold SyncSessions with a std::weak_ptr, except while calling a method #5815

RedBeard0531 · 2023-05-16T08:59:27Z

This avoids keeping SyncSessions alive due to lazy GC. This also matches the implementation technique of v11, which also held a weak_ptr for the same reasons.

What, How & Why?

This closes # ???

☑️ ToDos

📝 Changelog entry
📝 Compatibility label is updated or copied from previous entry
📝 Update COMPATIBILITY.md
🚦 Tests
🔀 Executed flexible sync tests locally if modifying flexible sync
📦 Updated internal package version in consuming package.jsons (if updating internal packages)
📱 Check the React Native/other sample apps work if necessary
📝 Public documentation PR created or is not necessary
💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

typescript definitions file is updated
jsdoc files updated

This avoids keeping SyncSessions alive due to lazy GC. This also matches the implementation technique of v11, which also held a weak_ptr for the same reasons.

RedBeard0531 · 2023-05-16T09:01:59Z

packages/realm/bindgen/js_spec.yml

@@ -17,3 +17,12 @@ classes:
      get_cpu_arch: () -> std::string
      # print: (const char* fmt, ...) # can't expose varargs directly. Could expose a fixed overload.

+  WeakSyncSession:
+    cppName: std::weak_ptr<SyncSession>


This is a bit of a hack, but I think it worked out reasonably well. I wanted to keep this contained to realm-js since I don't think other SDKs need to do this, and I'd rather not expose them to weak_ptr unless we have to. I also didn't want to have to start thinking about how we would handle weak_ptr generically in when generating code, so I kept it as a special case for SyncSession.

RedBeard0531 · 2023-05-16T09:02:46Z

packages/realm/bindgen/js_spec.yml

+    constructors:
+      weakCopyOf: '(shared: SharedSyncSession)'
+    methods:
+      rawDereference:
+        sig: '() const -> Nullable<SharedSyncSession>'
+        cppName: lock


None of these are directly used by code. Both are only used by the friendlier helpers in binding.ts at the very bottom of this PR.

If they're not intended for consumption by the SDK, could we find a way to declare that? Perhaps in the binding.ts by marking them as deprecated?

I'll see what I can do, but I doubt it. We could teach bindgen some way to do this, or look for the WeakSyncSession class by name (shudders), but I'd rather not do that. This class is used by exactly one other class, so the tradeoffs to prevent ourselves from using a less friendly API at the cost of higher complexity in the generator just don't seem right.

If you really don't like this pattern I could move the wrappers around this API from binding.WeakSyncSession to the implementation of the user-facing SyncSession class, but I personally like the pattern of using bindings.ts to augment the raw C++ APIs from TS.

I don't feel strongly for this - just wanted to avoid us accidentally relying on it, if it's not worth the complexity, I'm okay with leaving it as is.

like the pattern of using binding.ts to augment the raw C++ APIs from TS.

I agree, that seems best.

I didn't see a good technique that would let me add annotations or comments to both the static and instance methods. If you care, and are able to find such a technique, we can always add it later.

FYI, I'm about to push a change to move weaken() to be a method of binding.SyncSession rather than a static method on WeakSyncSession. If we are going to be augmenting the APIs from TS, we might as well make them nice APIs 😁

RedBeard0531 · 2023-05-16T09:04:15Z

packages/realm/bindgen/src/realm_js_jsi_helpers.h

@@ -134,7 +134,7 @@ REALM_NOINLINE inline jsi::Value toJsiException(jsi::Runtime& env, const std::ex

 [[noreturn]] REALM_NOINLINE inline void throwNullSharedPtrError(jsi::Runtime& env, const char* clsName)
 {
-    throw jsi::JSError(env, util::format("Attempting to use an instanace of $1 holding a null shared_ptr. "
+    throw jsi::JSError(env, util::format("Attempting to use an instanace of %1 holding a null shared_ptr. "


I don't know why my brain wants to use $1 placeholders rather than %1 with util::format, but I seem to make this typo a lot...

I have typed $ so many times that I know I have to stop and think

RedBeard0531 · 2023-05-16T09:06:00Z

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

  },
 });

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


I'm pretty sure we can kill this off now that we are using weak_ptr since a long-lived SyncSession won't keep the underlying C++ object alive anymore.

That would be great 🙏

RedBeard0531 · 2023-05-16T09:07:43Z

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

-    return this._internal;
+  public withInternal<Ret = void>(cb: (syncSession: binding.SyncSession) => Ret) {
+    return this.weakInternal.withDeref((syncSession) => {
+      assert(syncSession, "This SyncSession is no longer valid");


Note: this is the place that unconditionally throws from each method if the C++ object has been destroyed already. See the slack poll on #realm-js-team. This PR currently implements behavior (1) but I can easily change that if the consensus forms around a different option.

Let's try this and see if it becomes too painful to work with 👍

RedBeard0531 · 2023-05-16T09:11:50Z

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

-  remove({ internal, token }) {
-    return internal.unregisterProgressNotifier(token);
+  remove({ weakInternal, token }) {
+    weakInternal.withDeref((internal) => internal?.unregisterProgressNotifier(token));


Note: unlike other cases, this will NOT throw if the Session has been destroyed. removing() a listener from a destroyed session will silently do nothing. This seams reasonable to me since the listener was removed when the session was destroyed, and because remove() seems likely to be called as part of teardown, which may have caused the session to be destroyed already.

I'm a bit curious on why internal is able to be undefined / null, i.e. why you need the ?. here.

Never mind - read the implementation and now I get it :) I had to understand that the binding returns null while the withInternal method in the SDK threw.

Do you think it is worth a comment like // Note: not using the withInternal() helper, so internal can be null?

Nah - I just had to take a step back and read it properly 👍

kraenhansen · 2023-05-16T10:56:06Z

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

+    return this.weakInternal.withDeref((syncSession) => {
+      assert(syncSession, "This SyncSession is no longer valid");
+      return cb(syncSession);
+    });
  }

  /** @internal */
  constructor(internal: binding.SyncSession) {


I wonder if it's a problem that a strong binding.SyncSession is ever returned to the SDK? As long as it's not leaked, I think we'll be fine, but would it be easy to make the binding return weak sessions directly?

I think everywhere we touch a binding.SyncSession it basically immediately goes through here. In particular, I was concerned about these, and considered adding a call to weaken() immediately after getting a binding.SyncSession out of native, but I realized that it would still happen at exactly the same point, so didn't seem worth it.

realm-js/packages/realm/src/app-services/Sync.ts

Lines 74 to 87 in fb7de6e

static getAllSyncSessions(user: User): SyncSession[] {

return user.internal.allSessions.map((session) => new SyncSession(session));

}

static getSyncSession(user: User, partitionValue: PartitionValue): SyncSession | null {

validateSyncConfiguration({ user, partitionValue });

const config = toBindingSyncConfig({ user, partitionValue });

const path = user.app.internal.syncManager.pathForRealm(config, undefined);

const session = user.internal.sessionForOnDiskPath(path);

if (session) {

return new SyncSession(session);

} else {

return null;

}

}

After writing this comment, I understood that weaken will call the $resetSharedPtr which makes me less worried about a potential issue.

kraenhansen · 2023-05-16T11:06:59Z

Looks like the tests are generally happy 👍

packages/realm/bindgen/js_spec.yml

…on WeakSyncSession

coveralls · 2023-05-16T11:52:24Z

Coverage: 85.488% (-0.004%) from 85.493% when pulling be13e31 on ms/weak-sync-session into fb7de6e on main.

kneth · 2023-05-16T12:43:24Z

packages/realm/bindgen/src/realm_js_jsi_helpers.h

@@ -134,7 +134,7 @@ REALM_NOINLINE inline jsi::Value toJsiException(jsi::Runtime& env, const std::ex

 [[noreturn]] REALM_NOINLINE inline void throwNullSharedPtrError(jsi::Runtime& env, const char* clsName)
 {
-    throw jsi::JSError(env, util::format("Attempting to use an instanace of $1 holding a null shared_ptr. "
+    throw jsi::JSError(env, util::format("Attempting to use an instanace of %1 holding a null shared_ptr. "


I have typed $ so many times that I know I have to stop and think

* main: Remove incorrect info and adds to return description for compact() (#5798) Use `declare prop` rather than `prop!` with Object.declareProperty() Use Object.defineProperty rather than Object.defineProperties because it is faster Update importAppBefore to take an AppConfiguration (#5838) Adding `next` to install test matrix (#5819) Default session multiplexing to being disabled (#5831) Adding types for some "environment" options Exposing a defaultLogLevel context option Update pr-realm-js.yml Update PR workflow to pin the server version Update baas test server install script bump realm-core to v13.11.0 (#5814) Hold SyncSessions with a std::weak_ptr, except while calling a method (#5815) Update typescript.ts

Hold SyncSessions with a std::weak_ptr, except while calling a method

a956a23

This avoids keeping SyncSessions alive due to lazy GC. This also matches the implementation technique of v11, which also held a weak_ptr for the same reasons.

cla-bot bot added the cla: yes label May 16, 2023

github-actions bot assigned RedBeard0531 May 16, 2023

RedBeard0531 commented May 16, 2023

View reviewed changes

RedBeard0531 requested a review from kraenhansen May 16, 2023 09:32

RedBeard0531 marked this pull request as ready for review May 16, 2023 09:32

kraenhansen reviewed May 16, 2023

View reviewed changes

kraenhansen approved these changes May 16, 2023

View reviewed changes

elle-j approved these changes May 16, 2023

View reviewed changes

packages/realm/bindgen/js_spec.yml Outdated Show resolved Hide resolved

packages/realm/bindgen/js_spec.yml Outdated Show resolved Hide resolved

RedBeard0531 added 2 commits May 16, 2023 13:29

Make weaken() a method on SyncSession, rather than a static method …

c58031e

…on WeakSyncSession

use C++ style names even in js_spec.yml

be13e31

changelog

33e9c90

kneth approved these changes May 16, 2023

View reviewed changes

move change up to new "Breaking Changes" section in changelog

ccf06ef

RedBeard0531 merged commit 979400d into main May 16, 2023

RedBeard0531 deleted the ms/weak-sync-session branch May 16, 2023 13:08

Omrisnyk mentioned this pull request Nov 28, 2023

[Snyk] Fix for 5 vulnerabilities Omrisnyk/trinity-wallet#75

Open

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

Hold SyncSessions with a std::weak_ptr, except while calling a method #5815

Hold SyncSessions with a std::weak_ptr, except while calling a method #5815

RedBeard0531 commented May 16, 2023 •

edited

Loading

RedBeard0531 May 16, 2023

RedBeard0531 May 16, 2023

kraenhansen May 16, 2023

RedBeard0531 May 16, 2023

kraenhansen May 16, 2023 •

edited

Loading

RedBeard0531 May 16, 2023

RedBeard0531 May 16, 2023

RedBeard0531 May 16, 2023 •

edited

Loading

kneth May 16, 2023

RedBeard0531 May 16, 2023

kraenhansen May 16, 2023

RedBeard0531 May 16, 2023

kraenhansen May 16, 2023

RedBeard0531 May 16, 2023

kraenhansen May 16, 2023

kraenhansen May 16, 2023 •

edited

Loading

RedBeard0531 May 16, 2023

kraenhansen May 16, 2023

kraenhansen May 16, 2023 •

edited

Loading

RedBeard0531 May 16, 2023

kraenhansen May 16, 2023

kraenhansen commented May 16, 2023

coveralls commented May 16, 2023

kneth May 16, 2023

	static getAllSyncSessions(user: User): SyncSession[] {
	return user.internal.allSessions.map((session) => new SyncSession(session));
	}
	static getSyncSession(user: User, partitionValue: PartitionValue): SyncSession \| null {
	validateSyncConfiguration({ user, partitionValue });
	const config = toBindingSyncConfig({ user, partitionValue });
	const path = user.app.internal.syncManager.pathForRealm(config, undefined);
	const session = user.internal.sessionForOnDiskPath(path);
	if (session) {
	return new SyncSession(session);
	} else {
	return null;
	}
	}

Hold SyncSessions with a std::weak_ptr, except while calling a method #5815

Hold SyncSessions with a std::weak_ptr, except while calling a method #5815

Conversation

RedBeard0531 commented May 16, 2023 • edited Loading

What, How & Why?

☑️ ToDos

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 May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 May 16, 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

kraenhansen May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen commented May 16, 2023

coveralls commented May 16, 2023

Choose a reason for hiding this comment

RedBeard0531 commented May 16, 2023 •

edited

Loading

kraenhansen May 16, 2023 •

edited

Loading

RedBeard0531 May 16, 2023 •

edited

Loading

kraenhansen May 16, 2023 •

edited

Loading

kraenhansen May 16, 2023 •

edited

Loading