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

Address uses of @preconcurrency import Ably in ably-chat-swift#67 #1987

Closed
lawrence-forooghian opened this issue Oct 8, 2024 · 0 comments · Fixed by #1988
Closed

Address uses of @preconcurrency import Ably in ably-chat-swift#67 #1987

lawrence-forooghian opened this issue Oct 8, 2024 · 0 comments · Fixed by #1988
Assignees
Labels
enhancement New feature or improved functionality.

Comments

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Oct 8, 2024

ably/ably-chat-swift#67 introduces some @preconcurrency import Ably; let's figure out what we need to change in ably-cocoa to allow us to remove those.

┆Issue is synchronized with this Jira Task by Unito

@lawrence-forooghian lawrence-forooghian added the enhancement New feature or improved functionality. label Oct 8, 2024
lawrence-forooghian added a commit to ably/ably-chat-swift 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 to ably/ably-chat-swift 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
@maratal maratal self-assigned this Oct 8, 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.
Development

Successfully merging a pull request may close this issue.

2 participants