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

Implement the latest changes to room lifecycle spec #66

Open
lawrence-forooghian opened this issue Sep 26, 2024 · 0 comments
Open

Implement the latest changes to room lifecycle spec #66

lawrence-forooghian opened this issue Sep 26, 2024 · 0 comments
Assignees
Labels
enhancement New feature or improved functionality. room-lifecycle Related to room lifecycle (temporary label).

Comments

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Sep 26, 2024

  1. Many questions came up whilst implementing the spec; the conversation is ongoing on chat: room lifecycle specification ably/specification#200. Once these questions are answered, make any required code changes.
  2. Since creating the initial set of room lifecycle tasks, Andy has added further room lifecycle stuff to the spec (e.g. operation priorities, the INITIALIZING state). See what's new and implement.
  3. Some questions that were clarified but didn't result in spec changes:
    1. CHA-RL1h6 means to retry the whole DETACH operation, not just the channel detach

┆Issue is synchronized with this Jira Task by Unito

@lawrence-forooghian lawrence-forooghian added the enhancement New feature or improved functionality. label Sep 26, 2024
lawrence-forooghian added a commit that referenced this issue Sep 26, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44.

TODO btw i haven't put action descriptions in all of the test names because
they might end up really long

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

Resolves #53. (TODO move to next commit with the discontinuity points)

[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 Sep 26, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44.

TODO btw i haven't put action descriptions in all of the test names because
they might end up really long

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

Resolves #53. (TODO move to next commit with the discontinuity points)

[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 Sep 30, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44.

TODO btw i haven't put action descriptions in all of the test names because
they might end up really long

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

Resolves #53. (TODO move to next commit with the discontinuity points)

[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 Sep 30, 2024
Implements points CHA-RL4* from same spec as referenced in e70ee44.

TODO btw i haven't put action descriptions in all of the test names because
they might end up really long

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

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 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 lawrence-forooghian changed the title Implement outcomes of pending questions on spec Implement the latest changes to spec Oct 29, 2024
@lawrence-forooghian lawrence-forooghian changed the title Implement the latest changes to spec Implement the latest changes to room lifecycle spec Oct 29, 2024
lawrence-forooghian added a commit that referenced this issue Nov 6, 2024
Based on the spec referenced in 20f21c7. The RETRY part of this spec was
quite unclear, and I asked quite a few questions on the PR to understand
it better, so the behaviour implemented here is based on the spec plus
Andy’s answers to my questions (I’ve linked to the discussions in the
code and / or tests). Recently (i.e. after most of this commit was
already implemented, Andy has updated the spec with answers to these
questions, but in the interests of not dragging out the current task,
I’ll incorporate these updates in #66.

The internal triggering of the RETRY operation (as specified by
CHA-RL1h3 and CHA-RL4b9) will come in #50.

Resolves #51.
@lawrence-forooghian lawrence-forooghian added the room-lifecycle Related to room lifecycle (temporary label). label Nov 6, 2024
lawrence-forooghian added a commit that referenced this issue Nov 12, 2024
We use the implementation of the RELEASE operation provided by the room
lifecycle manager, and implement the spec points relating to room map
bookkeeping and releasing the underlying realtime channels.

Based on [1] at 6f0740a.

I have not implemented the spec points that relate to making sure that a
room fetch waits for any previous room with the same ID to finish
releasing; this is a part of the spec which is in flux (currently
implemented via the INITIALIZING status, which was added to the spec
after we started implementing the room lifecycle manager and hasn’t been
implemented in this SDK yet, and soon to be further changed in the spec
by making room-getting async). We can look at the current state of
things when we come to do #66.

[1] ably/specification#200
lawrence-forooghian added a commit that referenced this issue Nov 12, 2024
We use the implementation of the RELEASE operation provided by the room
lifecycle manager, and implement the spec points relating to room map
bookkeeping and releasing the underlying realtime channels.

Based on [1] at 6f0740a.

I have not implemented the spec points that relate to making sure that a
room fetch waits for any previous room with the same ID to finish
releasing; this is a part of the spec which is in flux (currently
implemented via the INITIALIZING status, which was added to the spec
after we started implementing the room lifecycle manager and hasn’t been
implemented in this SDK yet, and soon to be further changed in the spec
by making room-getting async). We can look at the current state of
things when we come to do #66.

Part of #47.

[1] ably/specification#200
lawrence-forooghian added a commit that referenced this issue Nov 12, 2024
We use the implementation of the RELEASE operation provided by the room
lifecycle manager, and implement the spec points relating to room map
bookkeeping and releasing the underlying realtime channels.

Based on [1] at 6f0740a.

I have not implemented the spec points that relate to making sure that a
room fetch waits for any previous room with the same ID to finish
releasing; this is a part of the spec which is in flux (currently
implemented via the INITIALIZING status, which was added to the spec
after we started implementing the room lifecycle manager and hasn’t been
implemented in this SDK yet, and soon to be further changed in the spec
by making room-getting async). We can look at the current state of
things when we come to do #66.

Part of #47.

[1] ably/specification#200
lawrence-forooghian added a commit that referenced this issue Nov 12, 2024
We use the implementation of the RELEASE operation provided by the room
lifecycle manager, and implement the spec points relating to room map
bookkeeping and releasing the underlying realtime channels.

Based on [1] at 6f0740a.

I have not implemented the spec points that relate to making sure that a
room fetch waits for any previous room with the same ID to finish
releasing; this is a part of the spec which is in flux (currently
implemented via the INITIALIZING status, which was added to the spec
after we started implementing the room lifecycle manager and hasn’t been
implemented in this SDK yet, and soon to be further changed in the spec
by making room-getting async). We can look at the current state of
things when we come to do #66.

Part of #47.

[1] ably/specification#200
lawrence-forooghian added a commit that referenced this issue Nov 12, 2024
Based on the spec referenced in 20f21c7. The RETRY part of this spec was
quite unclear, and I asked quite a few questions on the PR to understand
it better, so the behaviour implemented here is based on the spec plus
Andy’s answers to my questions (I’ve linked to the discussions in the
code and / or tests). Recently (i.e. after most of this commit was
already implemented, Andy has updated the spec with answers to these
questions, but in the interests of not dragging out the current task,
I’ll incorporate these updates in #66.

The internal triggering of the RETRY operation (as specified by
CHA-RL1h3 and CHA-RL4b9) will come in #50.

Resolves #51.
lawrence-forooghian added a commit that referenced this issue Nov 12, 2024
WIP updating to be based on 0e5ab98

Based on the spec referenced in 20f21c7. The RETRY part of this spec was
quite unclear, and I asked quite a few questions on the PR to understand
it better, so the behaviour implemented here is based on the spec plus
Andy’s answers to my questions (I’ve linked to the discussions in the
code and / or tests). Recently (i.e. after most of this commit was
already implemented, Andy has updated the spec with answers to these
questions, but in the interests of not dragging out the current task,
I’ll incorporate these updates in #66.

The internal triggering of the RETRY operation (as specified by
CHA-RL1h3 and CHA-RL4b9) will come in #50.

Resolves #51.
@lawrence-forooghian lawrence-forooghian self-assigned this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improved functionality. room-lifecycle Related to room lifecycle (temporary label).
Projects
None yet
Development

No branches or pull requests

1 participant