Consider the consequences of actor reentrancy #75
Labels
code-quality
Affects the developer experience when working in our codebase.
room-lifecycle
Related to room lifecycle (temporary label).
Comments
lawrence-forooghian
added
the
code-quality
lawrence-forooghian
added a commit
that referenced
this issue
Oct 1, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44. In addition to the TODOs added in the code (all of which refer either to existing GitHub issues or questions on the spec, for which we have #66 as a catch-all issue), I’ve also not done: - CHA-RL4a2 — I don’t understand the meaning of “has not yet successfully managed to attach its Realtime Channel”, asked about it in [1] - CHA-RL4b2 — seems redundant, asked about it in [2] - CHA-RL4b3, CHA-RL4b4 — seem redundant, asked about it in [3] - CHA-RL4b5, CHA-RL4b6, CHA-RL4b7 — these relate to transient disconnect timeouts, so will do them in #48 Something which I didn’t think about in 25e5052, and which I haven’t thought about here, is how actor reentrancy (i.e. the fact that when an actor-isolated method suspends via `await`, another actor-isolated method can be scheduled instead, which can potentially cause issues like state updates being interleaved in unexpected ways) might affect the room lifecycle manager. I would like to first of all implement the whole thing, specifically all of the spec points that might provide us with a mutual exclusion mechanism (i.e. the ones that tell us to wait until the current operation is complete), before assessing the situation. Have created #75 for this. Aside: I have not been consistent with the way that I’ve named the tests; the existing lifecycle manager test names have a part that describes the expected side effects. But I haven’t done that here because some of the spec points tested here have multiple side effects and the test names would become really long and hard to read. So for those ones I’ve only described the expected side effects inside the tests. I think we can live with the inconsistency for now. Part of #53. [1] https://github.com/ably/specification/pull/200/files#r1775552624 [2] https://github.com/ably/specification/pull/200/files#r1777212960 [3] https://github.com/ably/specification/pull/200/files#r1777365677
lawrence-forooghian
added a commit
that referenced
this issue
Oct 1, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44. In addition to the TODOs added in the code (all of which refer either to existing GitHub issues or questions on the spec, for which we have #66 as a catch-all issue), I’ve also not done: - CHA-RL4a2 — I don’t understand the meaning of “has not yet successfully managed to attach its Realtime Channel”, asked about it in [1] - CHA-RL4b2 — seems redundant, asked about it in [2] - CHA-RL4b3, CHA-RL4b4 — seem redundant, asked about it in [3] - CHA-RL4b5, CHA-RL4b6, CHA-RL4b7 — these relate to transient disconnect timeouts, so will do them in #48 Something which I didn’t think about in 25e5052, and which I haven’t thought about here, is how actor reentrancy (i.e. the fact that when an actor-isolated method suspends via `await`, another actor-isolated method can be scheduled instead, which can potentially cause issues like state updates being interleaved in unexpected ways) might affect the room lifecycle manager. I would like to first of all implement the whole thing, specifically all of the spec points that might provide us with a mutual exclusion mechanism (i.e. the ones that tell us to wait until the current operation is complete), before assessing the situation. Have created #75 for this. Aside: I have not been consistent with the way that I’ve named the tests; the existing lifecycle manager test names have a part that describes the expected side effects. But I haven’t done that here because some of the spec points tested here have multiple side effects and the test names would become really long and hard to read. So for those ones I’ve only described the expected side effects inside the tests. I think we can live with the inconsistency for now. Part of #53. [1] https://github.com/ably/specification/pull/200/files#r1775552624 [2] https://github.com/ably/specification/pull/200/files#r1777212960 [3] https://github.com/ably/specification/pull/200/files#r1777365677
lawrence-forooghian
added a commit
that referenced
this issue
Oct 1, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44. In addition to the TODOs added in the code (all of which refer either to existing GitHub issues or questions on the spec, for which we have #66 as a catch-all issue), I’ve also not done: - CHA-RL4a2 — I don’t understand the meaning of “has not yet successfully managed to attach its Realtime Channel”, asked about it in [1] - CHA-RL4b2 — seems redundant, asked about it in [2] - CHA-RL4b3, CHA-RL4b4 — seem redundant, asked about it in [3] - CHA-RL4b5, CHA-RL4b6, CHA-RL4b7 — these relate to transient disconnect timeouts, so will do them in #48 Something which I didn’t think about in 25e5052, and which I haven’t thought about here, is how actor reentrancy (i.e. the fact that when an actor-isolated method suspends via `await`, another actor-isolated method can be scheduled instead, which can potentially cause issues like state updates being interleaved in unexpected ways) might affect the room lifecycle manager. I would like to first of all implement the whole thing, specifically all of the spec points that might provide us with a mutual exclusion mechanism (i.e. the ones that tell us to wait until the current operation is complete), before assessing the situation. Have created #75 for this. Aside: I have not been consistent with the way that I’ve named the tests; the existing lifecycle manager test names have a part that describes the expected side effects. But I haven’t done that here because some of the spec points tested here have multiple side effects and the test names would become really long and hard to read. So for those ones I’ve only described the expected side effects inside the tests. I think we can live with the inconsistency for now. Part of #53. [1] https://github.com/ably/specification/pull/200/files#r1775552624 [2] https://github.com/ably/specification/pull/200/files#r1777212960 [3] https://github.com/ably/specification/pull/200/files#r1777365677
lawrence-forooghian
added a commit
that referenced
this issue
Oct 3, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44. In addition to the TODOs added in the code (all of which refer either to existing GitHub issues or questions on the spec, for which we have #66 as a catch-all issue), I’ve also not done: - CHA-RL4a2 — I don’t understand the meaning of “has not yet successfully managed to attach its Realtime Channel”, asked about it in [1] - CHA-RL4b2 — seems redundant, asked about it in [2] - CHA-RL4b3, CHA-RL4b4 — seem redundant, asked about it in [3] - CHA-RL4b5, CHA-RL4b6, CHA-RL4b7 — these relate to transient disconnect timeouts, so will do them in #48 Something which I didn’t think about in 25e5052, and which I haven’t thought about here, is how actor reentrancy (i.e. the fact that when an actor-isolated method suspends via `await`, another actor-isolated method can be scheduled instead, which can potentially cause issues like state updates being interleaved in unexpected ways) might affect the room lifecycle manager. I would like to first of all implement the whole thing, specifically all of the spec points that might provide us with a mutual exclusion mechanism (i.e. the ones that tell us to wait until the current operation is complete), before assessing the situation. Have created #75 for this. Aside: I have not been consistent with the way that I’ve named the tests; the existing lifecycle manager test names have a part that describes the expected side effects. But I haven’t done that here because some of the spec points tested here have multiple side effects and the test names would become really long and hard to read. So for those ones I’ve only described the expected side effects inside the tests. I think we can live with the inconsistency for now. Part of #53. [1] https://github.com/ably/specification/pull/200/files#r1775552624 [2] https://github.com/ably/specification/pull/200/files#r1777212960 [3] https://github.com/ably/specification/pull/200/files#r1777365677
lawrence-forooghian
added a commit
that referenced
this issue
Oct 8, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44. In addition to the TODOs added in the code (all of which refer either to existing GitHub issues or questions on the spec, for which we have #66 as a catch-all issue), I’ve also not done: - CHA-RL4a2 — I don’t understand the meaning of “has not yet successfully managed to attach its Realtime Channel”, asked about it in [1] - CHA-RL4b2 — seems redundant, asked about it in [2] - CHA-RL4b3, CHA-RL4b4 — seem redundant, asked about it in [3] - CHA-RL4b5, CHA-RL4b6, CHA-RL4b7 — these relate to transient disconnect timeouts, so will do them in #48 Something which I didn’t think about in 25e5052, and which I haven’t thought about here, is how actor reentrancy (i.e. the fact that when an actor-isolated method suspends via `await`, another actor-isolated method can be scheduled instead, which can potentially cause issues like state updates being interleaved in unexpected ways) might affect the room lifecycle manager. I would like to first of all implement the whole thing, specifically all of the spec points that might provide us with a mutual exclusion mechanism (i.e. the ones that tell us to wait until the current operation is complete), before assessing the situation. Have created #75 for this. Created [4] to address the `@preconcurrency import Ably` introduced by this commit. Aside: I have not been consistent with the way that I’ve named the tests; the existing lifecycle manager test names have a part that describes the expected side effects. But I haven’t done that here because some of the spec points tested here have multiple side effects and the test names would become really long and hard to read. So for those ones I’ve only described the expected side effects inside the tests. I think we can live with the inconsistency for now. Part of #53. [1] https://github.com/ably/specification/pull/200/files#r1775552624 [2] https://github.com/ably/specification/pull/200/files#r1777212960 [3] https://github.com/ably/specification/pull/200/files#r1777365677 [4] ably/ably-cocoa#1987
lawrence-forooghian
added a commit
that referenced
this issue
Oct 8, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44. In addition to the TODOs added in the code (all of which refer either to existing GitHub issues or questions on the spec, for which we have #66 as a catch-all issue), I’ve also not done: - CHA-RL4a2 — I don’t understand the meaning of “has not yet successfully managed to attach its Realtime Channel”, asked about it in [1] - CHA-RL4b2 — seems redundant, asked about it in [2] - CHA-RL4b3, CHA-RL4b4 — seem redundant, asked about it in [3] - CHA-RL4b5, CHA-RL4b6, CHA-RL4b7 — these relate to transient disconnect timeouts, so will do them in #48 Something which I didn’t think about in 25e5052, and which I haven’t thought about here, is how actor reentrancy (i.e. the fact that when an actor-isolated method suspends via `await`, another actor-isolated method can be scheduled instead, which can potentially cause issues like state updates being interleaved in unexpected ways) might affect the room lifecycle manager. I would like to first of all implement the whole thing, specifically all of the spec points that might provide us with a mutual exclusion mechanism (i.e. the ones that tell us to wait until the current operation is complete), before assessing the situation. Have created #75 for this. Created [4] to address the `@preconcurrency import Ably` introduced by this commit. Aside: I have not been consistent with the way that I’ve named the tests; the existing lifecycle manager test names have a part that describes the expected side effects. But I haven’t done that here because some of the spec points tested here have multiple side effects and the test names would become really long and hard to read. So for those ones I’ve only described the expected side effects inside the tests. I think we can live with the inconsistency for now. Part of #53. [1] https://github.com/ably/specification/pull/200/files#r1775552624 [2] https://github.com/ably/specification/pull/200/files#r1777212960 [3] https://github.com/ably/specification/pull/200/files#r1777365677 [4] ably/ably-cocoa#1987
lawrence-forooghian
added
the
room-lifecycle
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Something which I didn’t think about whilst doing the initial implementation of the room lifecycle manager is how actor reentrancy (i.e. the fact that when an actor-isolated method suspends via
await, another actor-isolated method can be scheduled instead, which can potentially cause issues like state updates being interleaved in unexpected ways) might affect the room lifecycle manager. I would like to first of all implement the whole thing, specifically all of the spec points that might provide us with a mutual exclusion mechanism (i.e. the ones that tell us to wait until the current operation is complete), before assessing the situation.┆Issue is synchronized with this Jira Task by Unito
The text was updated successfully, but these errors were encountered: