Clarify use of sync. Closes #2298 #2303
Conversation
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
feature/2298-fix-confused-and-updated-sync-situation
|
| Run status |
|
| Run duration | 09m 21s |
| Commit |
|
| Committer | Ricardo Iván Vieitez Parra |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
111
|
| View all changes introduced in this branch ↗︎ | |
corrideat
force-pushed
the
feature/2298-fix-confused-and-updated-sync-situation
branch
from
f8bb009 to
f992d55
Compare
|
Updated! |
corrideat
force-pushed
the
feature/2298-fix-confused-and-updated-sync-situation
branch
from
a858784 to
d71ad21
Compare
| const listener = () => { reset = true } | ||
| this.ephemeral.ondestroy = () => { | ||
| destroyed = true | ||
| sbp('okTurtle.events/off', CHELONIA_RESET, listener) | ||
| syncPromise.finally(() => { | ||
| if (reset) return | ||
| sbp('chelonia/contract/release', this.ephemeral.groupIdWhenMounted, { ephemeral: true }).catch(e => { | ||
| console.error('[PendingApproval.vue]: Error releasing contract', e) | ||
| }) | ||
| }) | ||
| } | ||
| sbp('okTurtle.events/on', CHELONIA_RESET, listener) | ||
| }, | ||
| beforeDestroy () { | ||
| this.ephemeral.ondestroy?.() |
There was a problem hiding this comment.
What is all of this new logic and why has it been added here?
I'm not sure this is a good idea... did you test this with multiple groups? It's possible that beforeDestroy could be called as a result of the group switcher switching to a different group. If that happens it would break the joining attempt on the new group that we're trying to join.
There was a problem hiding this comment.
This isn't really new logic, rather redefining the old logic that called /sync, which is unsafe to call.
It's possible that beforeDestroy could be called as a result of the group switcher switching to a different group
I don't see how that'd break anything. The onDestroy is there to call release, which is the correct thing to do after calling retain. If the view is destroyed, there's no point in keeping the group around (if it shouldn't be released, the reference count should indicate that). Note that before this only called sync and it also didn't affect the reference count.
did you test this with multiple groups?
No, I'm not sure how that'd be done as joining a group would open a new URL?
There was a problem hiding this comment.
No, I'm not sure how that'd be done as joining a group would open a new URL?
You'd create and join group1 as u1. Then u2 (in a separate container tab or browser) would send you an invite link to group2, but before u1 visits it, u2 would log out. Then u1 visits the link to join u2's group2 and get stuck on this pending page. Then they'd switch back to their own group. Then u2 would log back in.
I don't see how that'd break anything. The onDestroy is there to call release, which is the correct thing to do after calling retain. If the view is destroyed, there's no point in keeping the group around (if it shouldn't be released, the reference count should indicate that). Note that before this only called sync and it also didn't affect the reference count.
I see. Could you add a comment here as well then to explain where the "real" (persistent) retain happens on this group and why we're calling ephemeral retain here instead of sync?
There was a problem hiding this comment.
You'd create and join group1 as u1. [...]
I did this and nothing broke, as expected, since this is merely a UI concern.
Could you add a comment here as well then
Done. Also added a comment to the effect that vue files should not be managing references.
taoeffect
deleted the
feature/2298-fix-confused-and-updated-sync-situation
branch
Fix #2298