Realtime presence #651
Realtime presence #651
Conversation
WalkthroughAdds RTP2 presence: new PresenceMap and RealtimePresence modules, PresenceMessage encoding/decoding updates, channel and transport routing for PRESENCE and SYNC, Options.transport_params, connection close awaiting CLOSED via _when_state, connection-detail clearing on suspend/terminal states, and new unit and integration tests for presence. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RealtimeChannel
participant RealtimePresence
participant Transport
participant Server
participant PresenceMap
Client->>RealtimeChannel: presence.enter(data)
RealtimeChannel->>RealtimePresence: enter(data)
RealtimePresence->>RealtimePresence: build PresenceMessage (encrypt if configured)
alt Channel ATTACHED
RealtimePresence->>Transport: send PRESENCE (wire)
Transport->>Server: transmit PRESENCE
Server->>Transport: PRESENCE ack/delivered
Transport->>RealtimeChannel: route PRESENCE
RealtimeChannel->>RealtimePresence: set_presence(msg, is_sync=false)
RealtimePresence->>PresenceMap: put(msg)
PresenceMap-->>RealtimePresence: stored/present
RealtimePresence->>Client: emit 'enter'
else Not ATTACHED
RealtimePresence->>RealtimePresence: queue presence for later send
end
sequenceDiagram
participant Server
participant Transport
participant RealtimeChannel
participant RealtimePresence
participant PresenceMap
participant Client
Server->>Transport: send SYNC batch
Transport->>RealtimeChannel: route SYNC
RealtimeChannel->>RealtimePresence: set_presence(batch, is_sync=true, sync_channel_serial)
RealtimePresence->>PresenceMap: start_sync()
loop per message
RealtimePresence->>PresenceMap: put(message)
end
alt more SYNC batches
Note over Transport,PresenceMap: continue receiving batches
else SYNC complete
RealtimePresence->>PresenceMap: end_sync()
PresenceMap-->>RealtimePresence: residuals + absent members
RealtimePresence->>Client: emit synthesized LEAVE events for residuals/absent
PresenceMap->>PresenceMap: clear ABSENT and finalize sync
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/651/features
owenpearson
force-pushed
the
realtimepresence
branch
from
0e12e61 to
ac86265
Compare
github-actions
bot
temporarily deployed
to
staging/pull/651/features
ttypic
left a comment
ttypic
left a comment
There was a problem hiding this comment.
Looks great! Added couple of corner cases that we need to cover
owenpearson
force-pushed
the
realtimepresence
branch
from
ac86265 to
8acfb53
Compare
github-actions
bot
temporarily deployed
to
staging/pull/651/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
ably/realtime/connection.py (1)
90-95: Consider usingasyncio.get_running_loop()instead ofasyncio.get_event_loop().
asyncio.get_event_loop()is deprecated in Python 3.10+ when called from a coroutine context. Since_when_stateis called fromasync def close(), usingasyncio.get_running_loop()is more appropriate and future-proof.def _when_state(self, state: ConnectionState): if self.state == state: - fut = asyncio.get_event_loop().create_future() + fut = asyncio.get_running_loop().create_future() fut.set_result(None) return fut return self.once_async(state)ably/realtime/connectionmanager.py (2)
138-147: Duplicate state clearing logic betweenenact_state_changeandclose_impl.Connection state (connection_details, connection_id, connection_key, msg_serial) is cleared in both
enact_state_change(for SUSPENDED/CLOSED/FAILED) and inclose_impl. Sinceclose_implcallsnotify_state(ConnectionState.CLOSED)which eventually callsenact_state_change, the clearing inclose_impl(lines 189-194) is redundant and could be removed to avoid duplication and the attribute naming issues.Consider removing the explicit clearing in
close_implsinceenact_state_changewill handle it when transitioning to CLOSED state.Also applies to: 189-194
178-182: Background close task is fire-and-forget without error handling.The
asyncio.create_task(self.transport.close())call creates a background task but doesn't track it or handle potential exceptions. If the close message fails to send, the error will be silently lost.Consider adding a done callback or storing the task reference to log any errors:
if self.transport: # Try to send protocol CLOSE message in the background - asyncio.create_task(self.transport.close()) + close_task = asyncio.create_task(self.transport.close()) + close_task.add_done_callback(lambda t: t.exception() if not t.cancelled() else None) # Yield to event loop to give the close message a chance to send await asyncio.sleep(0)ably/realtime/realtime_channel.py (1)
578-590: Duplicate import statements forPresenceMessage.The
from ably.types.presence import PresenceMessageimport is duplicated in both the PRESENCE and SYNC handling branches. Consider hoisting this import to reduce redundancy.+ from ably.types.presence import PresenceMessage - elif action == ProtocolMessageAction.PRESENCE: + if action == ProtocolMessageAction.PRESENCE: # Handle PRESENCE messages - from ably.types.presence import PresenceMessage presence_messages = proto_msg.get('presence', []) decoded_presence = PresenceMessage.from_encoded_array(presence_messages, cipher=self.cipher) self.__presence.set_presence(decoded_presence, is_sync=False) elif action == ProtocolMessageAction.SYNC: # Handle SYNC messages (RTP18) - from ably.types.presence import PresenceMessage presence_messages = proto_msg.get('presence', []) decoded_presence = PresenceMessage.from_encoded_array(presence_messages, cipher=self.cipher) sync_channel_serial = proto_msg.get('channelSerial') self.__presence.set_presence(decoded_presence, is_sync=True, sync_channel_serial=sync_channel_serial)Alternatively, move the import to the top of the file if circular imports are not a concern.
ably/types/presence.py (1)
159-162: Redundantstr()call afterjson.dumps().
json.dumps()already returns a string, so the subsequentdata = str(data)on line 162 is unnecessary.if isinstance(data, (dict, list)): encoding.append('json') data = json.dumps(data) - data = str(data)test/ably/realtime/presencemap_test.py (1)
154-168: Consider movingimport base64to the top of the file.The
base64import is used in multiple test methods (lines 156, 171). Moving it to the module-level imports would be cleaner and more consistent with Python conventions.from datetime import datetime +import base64 from ably.realtime.presencemap import PresenceMap, _is_newertest/ably/realtime/realtimepresence_test.py (1)
654-655: Consider addressing the TODO comment about the 6-second sleep.The 6-second sleep is necessary due to the
remainPresentFor: 5000msserver-side behavior, but it significantly slows down the test suite. Consider marking this test for selective execution (e.g.,@pytest.mark.slow) or exploring if there's a way to mock the timing.Would you like me to open an issue to track optimizing this test's execution time?
ably/realtime/realtimepresence.py (4)
189-195: Consider renamingidparameter to avoid shadowing Python builtin.The parameter
idat line 191 shadows the Python builtinid()function. While it works fine here, using a name likemsg_idorpresence_idwould be cleaner.async def _enter_or_update_client( self, - id: str | None, + msg_id: str | None, client_id: str | None, data: Any, action: int ) -> None:Then update references at lines 243 and 657.
611-618: Redundantend_syncafterclear().At line 614,
self.members.clear()is called, which already resets_sync_in_progressto False (see presencemap.py line 339). The subsequent check at lines 617-618 will always be False.else: # RTP19a: No presence on channel, synthesize leaves for existing members self._synthesize_leaves(self.members.values()) self.members.clear() self.sync_complete = True - # Also end sync in case one was started - if self.members.sync_in_progress: - self.members.end_sync()
771-785: Consider making_action_name_impla module-level utility instead of monkey-patching.Monkey-patching
PresenceAction._action_namecreates implicit coupling and may be surprising to developers. Since it's a simple lookup, a module-level utility function would be cleaner:-# Helper for PresenceAction to convert action to string -def _action_name_impl(action: int) -> str: +def _action_name(action: int) -> str: """Convert presence action to string name.""" names = { PresenceAction.ABSENT: 'absent', PresenceAction.PRESENT: 'present', PresenceAction.ENTER: 'enter', PresenceAction.LEAVE: 'leave', PresenceAction.UPDATE: 'update', } return names.get(action, f'unknown({action})') - - -# Monkey-patch the helper onto PresenceAction -PresenceAction._action_name = staticmethod(_action_name_impl)Then update usages at lines 217, 222, and 592 to call
_action_name(action)directly.
423-431: Expose a public method on RealtimeChannel for waiting on state changes.Line 426 uses name mangling to access the private
__internal_state_emitter(_RealtimeChannel__internal_state_emitter). While this is the only place in the codebase where this pattern occurs, it creates unnecessary coupling betweenRealtimePresenceandRealtimeChannel's internal implementation. Consider adding a public or protected method likewait_for_state_change()onRealtimeChannelfor waiting on state transitions, which would provide a cleaner interface and better encapsulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
ably/realtime/connection.py(3 hunks)ably/realtime/connectionmanager.py(3 hunks)ably/realtime/presencemap.py(1 hunks)ably/realtime/realtime_channel.py(7 hunks)ably/realtime/realtimepresence.py(1 hunks)ably/transport/websockettransport.py(1 hunks)ably/types/options.py(3 hunks)ably/types/presence.py(4 hunks)test/ably/realtime/presencemap_test.py(1 hunks)test/ably/realtime/realtimepresence_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
ably/realtime/connection.py (3)
ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/realtime/realtime_channel.py (2)
state(713-715)state(718-719)ably/util/eventemitter.py (1)
once_async(169-182)
ably/realtime/presencemap.py (1)
ably/types/presence.py (11)
PresenceAction(24-29)PresenceMessage(32-237)is_synthesized(90-99)timestamp(78-79)parse_id(101-123)action(62-63)id(58-59)client_id(66-67)connection_id(70-71)data(74-75)extras(87-88)
ably/realtime/connectionmanager.py (5)
ably/realtime/connection.py (3)
state(109-111)state(120-121)close(61-68)ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/types/presence.py (1)
connection_id(70-71)ably/types/message.py (1)
connection_id(85-86)ably/types/options.py (1)
transport_params(287-288)
ably/types/presence.py (5)
ably/types/mixins.py (5)
EncodeDataMixin(29-130)encoding(35-36)encoding(39-43)decode(46-126)from_encoded_array(129-130)ably/types/typedbuffer.py (4)
TypedBuffer(39-104)from_obj(57-90)buffer(93-94)decode(100-104)ably/util/crypto.py (4)
CipherData(122-131)encrypt(92-98)cipher_type(118-119)encoding_str(130-131)ably/util/exceptions.py (1)
AblyException(9-84)ably/types/message.py (6)
id(77-78)id(81-82)encrypt(100-116)data(69-70)extras(97-98)from_encoded(181-206)
test/ably/realtime/realtimepresence_test.py (7)
ably/types/channelstate.py (1)
ChannelState(8-15)ably/realtime/realtime_channel.py (7)
presence(733-735)get(768-793)subscribe(283-335)state(713-715)state(718-719)attach(164-202)detach(222-269)ably/types/presence.py (2)
PresenceAction(24-29)action(62-63)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)ably/realtime/realtimepresence.py (5)
get(383-438)subscribe(457-486)enter(89-106)leave(129-145)update(108-127)ably/realtime/presencemap.py (1)
get(99-109)ably/types/channelsubscription.py (1)
channel(19-20)
test/ably/realtime/presencemap_test.py (2)
ably/realtime/presencemap.py (9)
PresenceMap(62-341)_is_newer(15-59)put(111-157)remove(159-204)values(206-216)clear(331-341)sync_in_progress(95-97)start_sync(249-263)end_sync(265-315)ably/types/presence.py (13)
PresenceAction(24-29)PresenceMessage(32-237)id(58-59)connection_id(70-71)client_id(66-67)action(62-63)is_synthesized(90-99)parse_id(101-123)member_key(82-84)data(74-75)timestamp(78-79)to_encoded(147-203)from_encoded_array(233-237)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.8)
- GitHub Check: check (3.9)
- GitHub Check: check (3.14)
- GitHub Check: check (3.13)
- GitHub Check: check (3.11)
- GitHub Check: check (3.12)
- GitHub Check: check (3.10)
🔇 Additional comments (34)
ably/transport/websockettransport.py (1)
180-187: LGTM!The addition of
PRESENCEandSYNCactions to the channel message routing is correct. This enables the channel layer to handle presence updates and synchronization protocol events as part of the RTP2 presence support.ably/types/options.py (1)
35-35: LGTM!The
transport_paramsoption is cleanly implemented following the existing patterns in the class. The default empty dict and read-only property are appropriate for this use case.Also applies to: 99-99, 286-288
ably/realtime/realtime_channel.py (2)
139-141: LGTM!The deferred import of
RealtimePresenceinside__init__is an acceptable pattern to avoid circular import issues between the channel and presence modules.
600-601: LGTM!The
has_presenceparameter is correctly propagated through_notify_stateto the presence handler viaact_on_channel_state. This enables proper RTP5 presence state management based on channel lifecycle events.Also applies to: 639-640
ably/types/presence.py (2)
90-123: LGTM!The
is_synthesized()andparse_id()methods correctly implement the RTP2b1 and RTP2b2 spec requirements for detecting synthesized leave events and parsing message IDs for comparison. The error handling inparse_id()appropriately raisesValueErrorfor invalid formats.
232-237: LGTM!The
from_encoded_arraystatic method provides a clean batch decoding interface that aligns with the existing pattern used for messages.ably/realtime/connectionmanager.py (1)
144-146: Fix inconsistent attribute reference inclose_impl().Line 192 uses
self.__connection_id, which is not initialized in__init__and doesn't follow the pattern used inenact_state_change()at line 145. Changeself.__connection_id = Nonetoself.connection_id = Noneto match the initialized public attribute and maintain consistency.Likely an incorrect or invalid review comment.
test/ably/realtime/presencemap_test.py (4)
1-12: LGTM - Clean module setup with appropriate imports.The module docstring clearly states the purpose (RTP2 specification testing), and imports are well-organized.
237-330: LGTM - Comprehensive newness comparison tests.Good coverage of RTP2b1 (timestamp comparison for synthesized messages) and RTP2b2 (msgSerial/index comparison for normal messages), including edge cases like equal timestamps and equal serial/index values.
333-563: LGTM - Solid basic operations test coverage.Tests properly verify RTP2 specification requirements including newness checks, action normalization (ENTER/UPDATE stored as PRESENT), and filtering. The direct
_mapaccess at line 494 for testing ABSENT member behavior is acceptable for unit test purposes.
566-732: LGTM - Comprehensive SYNC operation tests.Excellent coverage of RTP18/RTP19 sync lifecycle including residual member tracking, ABSENT state handling during sync, and idempotent
start_syncbehavior. The tests correctly verify that residual members are identified for synthesized leave events.ably/realtime/presencemap.py (7)
15-59: LGTM - Correct RTP2b newness comparison implementation.The logic correctly handles:
- RTP2b1: Synthesized message comparison by timestamp
- RTP2b1a: Equal timestamps favor the incoming message
- RTP2b2: Normal message comparison by msgSerial then index
The None timestamp handling is well thought out.
72-97: LGTM - Clean constructor with proper dependency injection.Good use of optional
is_newer_fnandlogger_instanceparameters for testability and flexibility.
111-157: LGTM - Correct RTP2d implementation.The put method correctly:
- Normalizes ENTER/UPDATE/PRESENT to PRESENT
- Creates an immutable copy rather than mutating the input
- Removes members from residual tracking during sync
- Applies newness checks before storing
159-204: LGTM - Correct RTP2h implementation.The remove method correctly differentiates between sync and non-sync behavior:
- RTP2h1: Outside sync, removes member from map
- RTP2h2: During sync, marks member as ABSENT for later cleanup
206-247: LGTM - Clean filtering implementation.Both
values()andlist()correctly exclude ABSENT members, withlist()providing additional clientId/connectionId filtering per RTP11.
249-315: LGTM - Correct RTP18/RTP19 sync lifecycle implementation.The sync methods correctly:
- Capture residual members at sync start for later reconciliation
- Handle ABSENT members and residual members at sync end
- Return both lists for synthesized leave event emission
- Use try/except for callback invocation to prevent one failure from blocking others
331-341: LGTM - Complete state reset for RTP5a.The clear method properly resets all internal state, including pending sync callbacks.
test/ably/realtime/realtimepresence_test.py (7)
18-28: LGTM - Useful test helper for simulating connection suspension.The
force_suspendedhelper correctly sequences the disconnection and suspension states for testing connection recovery scenarios.
31-175: LGTM - Solid basic presence operation tests.Good coverage of RTP8 (enter), RTP9 (update), RTP10 (leave), and error handling for anonymous clients. The use of
asyncio.sleepfor timing in integration tests is acceptable.
177-261: LGTM - Good get() functionality tests.Properly verifies RTP11a (get returns members) and RTP11b (implicit attach on unattached channel).
264-329: LGTM - Good subscribe functionality tests.Properly verifies RTP6d (implicit attach on subscribe) and action field handling.
331-409: LGTM - Good enterClient/updateClient/leaveClient tests.Properly uses wildcard clientId for RTP14/RTP15 multi-client presence testing.
412-526: LGTM - Good connection lifecycle tests.Tests cover important edge cases including entering before connection is established, re-entering after reconnect, and error handling for closed connections.
683-829: LGTM - Comprehensive SYNC behavior tests.Excellent coverage of RTP5f (presence preserved during SUSPENDED), RTP11d (get behavior during SUSPENDED), and RTP15b (refresh on reattach). The tests properly verify that only changed members trigger events after reconnection.
ably/realtime/realtimepresence.py (9)
28-44: LGTM - Correct clientId handling helpers.The
_get_client_idfallback chain and_is_anonymous_or_wildcardcheck are well-designed, correctly handling the case where clientId might not be known until after connection.
62-87: LGTM - Proper dual PresenceMap setup for RTP2/RTP17.Correctly maintains separate presence maps:
members: All channel members keyed byconnectionId:clientId_my_members: Own client's entries keyed byclientIdonly (for RTP17 re-entry tracking)
89-145: LGTM - Consistent anonymous client validation.All three public methods (enter, update, leave) correctly validate that the client has a valid clientId before proceeding.
275-348: LGTM - Correct RTP10e implementation.The leave method correctly avoids attaching the channel just to send a leave message, and properly handles all channel states.
350-381: LGTM - Clean async queuing pattern.The
_queue_presencemethod correctly uses a Future to allow callers to await the actual send completion after channel attaches.
457-511: LGTM - Clean subscription handling.Subscribe correctly implements RTP6d (implicit attach) using
asyncio.create_taskfor non-blocking operation. Argument parsing follows the standard pattern.
626-669: LGTM - Correct RTP17g/RTP17g1 re-entry implementation.The re-entry logic correctly:
- Iterates own members for re-entry
- Suppresses message ID when connectionId has changed
- Uses non-blocking tasks for re-entry operations
Note: RTP17e error emission (channel state change with error) is documented as not fully implemented, which is acceptable for this phase.
671-699: LGTM - Safe pending message handling.Good pattern of swapping the pending list before processing to prevent issues with concurrent modifications during async operations.
719-744: LGTM - Correct RTP5 state handling.Properly differentiates between:
- ATTACHED: triggers attach handling
- DETACHED/FAILED: clears all state (RTP5a)
- SUSPENDED: preserves members but fails pending (RTP5f)
owenpearson
force-pushed
the
realtimepresence
branch
from
8acfb53 to
aa861f1
Compare
github-actions
bot
temporarily deployed
to
staging/pull/651/features
owenpearson
force-pushed
the
realtimepresence
branch
from
aa861f1 to
5b72275
Compare
github-actions
bot
temporarily deployed
to
staging/pull/651/features
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
ably/realtime/realtimepresence.py (1)
579-587: Replacedatetime.utcnow()with timezone‑aware UTC (datetime.now(timezone.utc)).Both synthesized-leave paths use
datetime.utcnow(), which is deprecated in Python 3.12+ and produces naive datetimes. For better future‑compatibility and correctness, prefer timezone‑aware UTC:-from datetime import datetime +from datetime import datetime, timezone @@ - timestamp=datetime.utcnow() + timestamp=datetime.now(timezone.utc) @@ - timestamp=datetime.utcnow() + timestamp=datetime.now(timezone.utc)Please confirm against your supported Python versions and existing datetime usage elsewhere in the repo to keep behavior consistent.
Also applies to: 708-716
🧹 Nitpick comments (7)
ably/realtime/connection.py (1)
90-95: Useasyncio.get_running_loop()instead ofasyncio.get_event_loop().
asyncio.get_event_loop()is deprecated in Python 3.10+ and may emit warnings or behave unexpectedly in non-main threads. Since_when_stateis called from async context (viaclose()), useasyncio.get_running_loop()which is the recommended approach.def _when_state(self, state: ConnectionState): if self.state == state: - fut = asyncio.get_event_loop().create_future() + fut = asyncio.get_running_loop().create_future() fut.set_result(None) return fut return self.once_async(state)test/ably/realtime/realtimepresence_test.py (1)
697-699: Consider replacing the 6-second sleep with event-based waiting.This long fixed sleep waits for the
remainPresentFortimeout, which could cause test flakiness or slow test execution. The TODO comment acknowledges this issue. Consider using an event-based approach or reducing theremainPresentForvalue in the test to minimize wait time.ably/realtime/realtime_channel.py (1)
578-590: Consider consolidating thePresenceMessageimport.The
PresenceMessageimport appears twice (lines 580 and 586). You could move it to the top of the handler block or check if it can be safely added to the module-level imports.elif action == ProtocolMessageAction.PRESENCE: # Handle PRESENCE messages from ably.types.presence import PresenceMessage presence_messages = proto_msg.get('presence', []) decoded_presence = PresenceMessage.from_encoded_array(presence_messages, cipher=self.cipher) self.__presence.set_presence(decoded_presence, is_sync=False) elif action == ProtocolMessageAction.SYNC: # Handle SYNC messages (RTP18) - from ably.types.presence import PresenceMessage presence_messages = proto_msg.get('presence', []) decoded_presence = PresenceMessage.from_encoded_array(presence_messages, cipher=self.cipher) sync_channel_serial = proto_msg.get('channelSerial') self.__presence.set_presence(decoded_presence, is_sync=True, sync_channel_serial=sync_channel_serial)ably/realtime/presencemap.py (1)
137-138: Clarify the fallback branch for non-ENTER/UPDATE/PRESENT actions.The
elsebranch stores the item as-is when action is not ENTER, UPDATE, or PRESENT. Since LEAVE should useremove()and ABSENT is internal, this branch appears unreachable in normal operation. Consider adding a log warning or raising an error for unexpected actions.else: - item_to_store = item + self._logger.warning(f"PresenceMap.put: unexpected action {item.action}, storing as-is") + item_to_store = itemtest/ably/realtime/presencemap_test.py (2)
130-155: Make JSON expectations schema-based, not exact-string, to avoid brittle tests.Both
test_to_encoded_with_dict_dataandtest_to_encoded_with_list_dataassert exact JSON strings. Any change injson.dumpsoptions (e.g. separators, key ordering) would break these tests without changing semantics.Consider asserting via
json.loadsinstead, e.g.:+import json @@ - encoded = msg.to_encoded() - assert encoded['data'] == '{"key": "value", "number": 42}' + encoded = msg.to_encoded() + assert json.loads(encoded['data']) == {'key': 'value', 'number': 42} assert encoded['encoding'] == 'json'and similarly for the list case.
491-499: AccessingPresenceMap._mapdirectly in tests couples you to internals.
test_values_excludes_absentwrites directly toself.presence_map._map. This works but makes tests fragile if the implementation changes its internal storage.If possible, prefer using public behavior (e.g. a dedicated helper or a public method to insert ABSENT entries during sync) rather than relying on
_mapinternals.ably/realtime/realtimepresence.py (1)
471-487:subscribeargument handling is minimal but acceptable; docstring over‑states supported forms.
subscribecurrently supports:
subscribe(listener)subscribe(event, listener)The docstring also mentions
(events, listener)whereeventsis a list. That will only work ifEventEmitter.onalready supports a list as its first argument; there’s no explicit branching here.If multi‑event subscribe is required by the public API, consider either:
- Explicitly handling
Sequence[str]vsstr, or- Narrowing the docstring to only the currently supported forms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
ably/realtime/connection.py(3 hunks)ably/realtime/connectionmanager.py(3 hunks)ably/realtime/presencemap.py(1 hunks)ably/realtime/realtime_channel.py(7 hunks)ably/realtime/realtimepresence.py(1 hunks)ably/transport/websockettransport.py(1 hunks)ably/types/options.py(3 hunks)ably/types/presence.py(4 hunks)test/ably/realtime/presencemap_test.py(1 hunks)test/ably/realtime/realtimepresence_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ably/realtime/connectionmanager.py
- ably/types/options.py
- ably/types/presence.py
🧰 Additional context used
🧬 Code graph analysis (6)
test/ably/realtime/realtimepresence_test.py (8)
ably/types/channelstate.py (1)
ChannelState(8-15)ably/types/presence.py (2)
PresenceAction(24-29)action(62-63)ably/util/exceptions.py (1)
AblyException(9-84)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)test/ably/utils.py (1)
BaseAsyncTestCase(36-51)ably/realtime/connectionmanager.py (2)
request_state(470-493)notify_state(568-614)ably/realtime/realtimepresence.py (5)
get(383-438)subscribe(457-486)enter(89-106)leave(129-145)update(108-127)ably/realtime/presencemap.py (1)
get(99-109)
ably/realtime/realtime_channel.py (4)
ably/realtime/realtimepresence.py (4)
RealtimePresence(47-768)get(383-438)set_presence(513-593)act_on_channel_state(719-744)ably/types/flags.py (2)
has_flag(18-19)Flag(4-15)ably/types/presence.py (4)
action(62-63)PresenceMessage(32-237)get(253-263)from_encoded_array(233-237)ably/transport/websockettransport.py (1)
ProtocolMessageAction(34-56)
ably/realtime/presencemap.py (2)
ably/types/presence.py (8)
PresenceAction(24-29)PresenceMessage(32-237)is_synthesized(90-99)parse_id(101-123)get(253-263)id(58-59)client_id(66-67)connection_id(70-71)ably/realtime/realtimepresence.py (1)
get(383-438)
ably/realtime/realtimepresence.py (5)
ably/realtime/presencemap.py (10)
PresenceMap(62-341)get(99-109)sync_in_progress(95-97)wait_sync(317-329)start_sync(249-263)remove(159-204)put(111-157)end_sync(265-315)values(206-216)clear(331-341)ably/types/channelstate.py (1)
ChannelState(8-15)ably/types/presence.py (13)
PresenceAction(24-29)PresenceMessage(32-237)client_id(66-67)member_key(82-84)data(74-75)id(58-59)action(62-63)encrypt(125-145)to_encoded(147-203)get(253-263)connection_id(70-71)is_synthesized(90-99)timestamp(78-79)ably/util/eventemitter.py (5)
EventEmitter(25-186)once_async(169-182)on(45-87)off(134-167)_emit(184-186)ably/util/exceptions.py (1)
AblyException(9-84)
test/ably/realtime/presencemap_test.py (2)
ably/realtime/presencemap.py (10)
PresenceMap(62-341)_is_newer(15-59)put(111-157)get(99-109)remove(159-204)values(206-216)clear(331-341)sync_in_progress(95-97)start_sync(249-263)end_sync(265-315)ably/types/presence.py (13)
PresenceMessage(32-237)id(58-59)connection_id(70-71)client_id(66-67)action(62-63)is_synthesized(90-99)parse_id(101-123)member_key(82-84)data(74-75)timestamp(78-79)to_encoded(147-203)from_encoded_array(233-237)get(253-263)
ably/realtime/connection.py (4)
ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/realtime/realtime_channel.py (2)
state(713-715)state(718-719)ably/realtime/connectionmanager.py (1)
state(750-751)ably/util/eventemitter.py (1)
once_async(169-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.13)
- GitHub Check: check (3.10)
- GitHub Check: check (3.14)
- GitHub Check: check (3.8)
- GitHub Check: check (3.12)
- GitHub Check: check (3.9)
- GitHub Check: check (3.11)
🔇 Additional comments (15)
ably/transport/websockettransport.py (1)
180-187: LGTM! Correct routing of PRESENCE and SYNC protocol messages.Extending the channel message routing to include
PRESENCEandSYNCactions is necessary for the new presence functionality. This properly delegates these protocol messages toon_channel_messagewhere they can be processed by individual channels.ably/realtime/connection.py (1)
61-68: LGTM! Graceful close handling.The updated
close()method correctly waits for the connection to reach theCLOSEDstate using the new_when_statehelper. This addresses the requirement from issue #649 to ensure the connection properly completes the close handshake before returning.test/ably/realtime/realtimepresence_test.py (2)
37-55: LGTM! Well-structured test fixtures.The fixture pattern with
autouse=Trueand proper teardown usingyieldensures clean resource management. The parameterization for binary protocol testing provides good coverage.
567-628: LGTM! Comprehensive auto re-entry test coverage.The tests properly verify RTP5f, RTP17, and RTP17i requirements for automatic presence re-entry after suspension. The test logic correctly validates that members remain in the presence set after reconnection.
ably/realtime/realtime_channel.py (3)
139-141: LGTM! Lazy import to avoid circular dependency.The import of
RealtimePresenceinside__init__is a valid pattern to prevent circular import issues betweenrealtime_channel.pyandrealtimepresence.py.
600-601: LGTM! Presence integration with channel state notifications.The updated
_notify_statesignature properly propagateshas_presenceinformation, and the call toact_on_channel_stateensures presence correctly responds to channel lifecycle changes per RTP5.Also applies to: 639-640
732-735: LGTM! Clean public accessor for presence.The
presenceproperty follows the pattern of other channel properties and provides access to theRealtimePresenceinstance for the channel.ably/realtime/presencemap.py (4)
15-59: LGTM! Correct implementation of RTP2b newness comparison.The
_is_newerfunction properly handles:
- RTP2b1: Synthesized message comparison by timestamp
- RTP2b1a: Equal timestamps favor the incoming message (
>=)- RTP2b2: Normal message comparison by msgSerial then index
The null-safety checks for timestamps are also well handled.
265-315: LGTM! Correct SYNC completion handling per RTP18/RTP19.The
end_syncmethod properly:
- Collects and removes ABSENT members (RTP2h2b)
- Identifies residual members for synthesized LEAVE events (RTP19)
- Safely invokes callbacks with error handling
- Clears sync state atomically
317-329: LGTM! Clean sync waiting implementation.The
wait_syncmethod correctly handles both cases: immediate callback when no sync is in progress, and deferred callback otherwise.
331-341: LGTM! Complete state reset for channel detach/fail scenarios.The
clearmethod properly resets all internal state including pending callbacks, preventing memory leaks when channels are detached or fail (RTP5a).test/ably/realtime/presencemap_test.py (1)
734-738: Good coverage ofstart_syncidempotency with_residual_members.
test_start_sync_multiple_timesnicely locks in the contract that repeatedstart_sync()calls during an active sync preserve the original residual snapshot. This will help prevent regressions in PresenceMap’s sync lifecycle behavior.ably/realtime/realtimepresence.py (3)
240-273: Connection-state gate looks good and matches the queueing use case.The connection-state whitelist in
_enter_or_update_client(CONNECTING,CONNECTED,DISCONNECTED) aligns with queuing semantics—allowing presence calls when disconnected but not when the connection is closing/closed. Together with_send_presencedelegating toconnection_manager.send_protocol_message, this should cover the “queue messages while attached but disconnected” scenario raised in earlier review discussion.
367-382: Pending presence queueing and flushing logic looks sound.The
_queue_presence/_send_pending_presence/_fail_pending_presencetrio gives a clear lifecycle:
- Queue encoded presence + a Future.
- On attach, send all pending in one protocol message, then resolve Futures.
- On error/DETACHED/FAILED/SUSPENDED, reject all pending Futures with an AblyException (or provided error).
The pattern of swapping out
self._pending_presencebefore awaiting send avoids races with new messages arriving mid‑flush, and error handling correctly ensures Futures are not left unresolved.Also applies to: 671-700
595-625: Attach handling and pending‑presence flush flow is coherent.
on_attachedcorrectly:
- Starts sync or synthesizes leaves and clears state depending on
has_presence.- Re‑enters own members via
_ensure_my_members_present.- Schedules
_send_pending_presence()to flush any queued messages.The sequencing (sync setup → local re‑entry → pending flush) matches the intended presence life‑cycle and should behave well with the PresenceMap semantics you’ve tested.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
test/ably/realtime/realtimepresence_test.py (3)
20-30: UseConnectionStateenum values instead of string literals.The
_when_statemethod compares againstConnectionStateenum values, but string literals are passed here. This will cause the equality check to fail.async def force_suspended(client): client.connection.connection_manager.request_state(ConnectionState.DISCONNECTED) - await client.connection._when_state('disconnected') + await client.connection._when_state(ConnectionState.DISCONNECTED) client.connection.connection_manager.notify_state( ConnectionState.SUSPENDED, AblyException("Connection to server unavailable", 400, 80002) ) - await client.connection._when_state('suspended') + await client.connection._when_state(ConnectionState.SUSPENDED)
177-183: Replaceself.fail()withpytest.fail()for pytest-based tests.
BaseAsyncTestCasedoesn't inherit fromunittest.TestCaseand lacks afail()method. This will raiseAttributeErrorat runtime.try: await channel.presence.enter('data') - self.fail('Should have raised exception for anonymous client') + pytest.fail('Should have raised exception for anonymous client') except Exception as e: assert 'clientId must be specified' in str(e)
557-562: Replaceself.fail()withpytest.fail()here as well.Same issue as above -
self.fail()will raiseAttributeError.try: await channel.presence.enter_client('client1', 'data') - self.fail('Should have raised exception for closed connection') + pytest.fail('Should have raised exception for closed connection') except Exception as e:ably/realtime/realtimepresence.py (4)
89-187: Fix reversed AblyException arguments throughout this file.All
AblyExceptioninstantiations in this file have the HTTP status code and Ably error code arguments reversed. The signature isAblyException(message, status_code, code, cause=None), wherestatus_codeis the HTTP status (e.g., 400) andcodeis the Ably error code (e.g., 40012, 90001).Current pattern:
AblyException('message', 40012, 400)❌
Correct pattern:AblyException('message', 400, 40012)✅This affects lines: 103, 124, 142, 219, 235, 272, 297, 311, 342, 346, 408, 420, 430, 764.
Based on past review comments.
Apply this diff to fix the enter/update/leave methods:
if _is_anonymous_or_wildcard(self): raise AblyException( 'clientId must be specified to enter a presence channel', - 40012, 400 + 400, 40012 )if _is_anonymous_or_wildcard(self): raise AblyException( 'clientId must be specified to update presence data', - 40012, 400 + 400, 40012 )if _is_anonymous_or_wildcard(self): raise AblyException( 'clientId must have been specified to enter or leave a presence channel', - 40012, 400 + 400, 40012 )
586-586: Replace deprecateddatetime.utcnow()with timezone-aware UTC datetime.
datetime.utcnow()is deprecated in Python 3.12+. Usedatetime.now(timezone.utc)for compatibility.Based on past review comments.
Update the import at line 12:
-from datetime import datetime +from datetime import datetime, timezoneThen apply this diff:
timestamp=datetime.utcnow() + timestamp=datetime.now(timezone.utc)(Note: Also fix line 715 with the same change.)
715-715: Replace deprecateddatetime.utcnow()with timezone-aware UTC datetime.Same deprecation issue as line 586. Use
datetime.now(timezone.utc)instead.Based on past review comments.
735-744: Callend_sync()before clearing members to prevent indefinite hangs.When the channel transitions to DETACHED, FAILED, or SUSPENDED during a SYNC, any caller blocked in
get(wait_for_sync=True)will hang indefinitely. The issue:
get()calls_wait_for_sync()which registers a callback viamembers.wait_sync()act_on_channel_state()callsmembers.clear()(line 738) which clears_sync_complete_callbackswithout invoking them- The Future in
_wait_for_sync()never resolvesIn contrast,
on_attached(has_presence=False)correctly callsend_sync()(line 618) after clearing to unblock waiters.Based on past review comments.
Apply this diff to unblock waiters before clearing:
elif state in (ChannelState.DETACHED, ChannelState.FAILED): # RTP5a: Clear maps and fail pending self._my_members.clear() + # End any in-progress sync before clearing to unblock waiters + if self.members.sync_in_progress: + self.members.end_sync() self.members.clear() self.sync_complete = False self._fail_pending_presence(error) elif state == ChannelState.SUSPENDED: # RTP5f: Fail pending but keep members, reset sync state + # End any in-progress sync to unblock waiters + if self.members.sync_in_progress: + self.members.end_sync() self.sync_complete = False # Sync state is no longer valid self._fail_pending_presence(error)
🧹 Nitpick comments (5)
test/ably/realtime/realtimepresence_test.py (1)
696-699: Address the long sleep or add explanation.The 6-second sleep is problematic for test performance. The TODO comment acknowledges this needs attention. Consider using an event-based wait or reducing the
remainPresentFortimeout in the test setup.test/ably/realtime/presencemap_test.py (2)
335-344: Minor: Consider usingasync def setupfor consistency with other test files.While pytest fixtures work with sync functions, the other test files in this PR use
async def setup. For consistency, consider making this async even if it doesn't need to be.@pytest.fixture(autouse=True) - def setup(self): + async def setup(self): """Set up test fixtures.""" self.presence_map = PresenceMap( member_key_fn=lambda msg: msg.member_key ) yield
491-502: Consider testing ABSENT action via the public API rather than internal access.Directly accessing
self.presence_map._mapbypasses the public interface. Consider using theremovemethod during sync to create ABSENT members, which would make the test more representative of actual usage.ably/types/presence.py (1)
159-162: Remove redundantstr()conversion.
json.dumps()already returns a string, so thedata = str(data)on line 162 is redundant.if isinstance(data, (dict, list)): encoding.append('json') data = json.dumps(data) - data = str(data)ably/realtime/realtimepresence.py (1)
771-785: Consider defining_action_namein the PresenceAction class.Monkey-patching
_action_nameontoPresenceActionat module load time works but is unconventional. IfPresenceActionis defined inably/types/presence.py(which is controlled by this project), consider adding this as a proper static method or class method there instead.This would improve discoverability and avoid the surprise of a dynamically added method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
ably/realtime/connection.py(3 hunks)ably/realtime/connectionmanager.py(3 hunks)ably/realtime/presencemap.py(1 hunks)ably/realtime/realtime_channel.py(7 hunks)ably/realtime/realtimepresence.py(1 hunks)ably/transport/websockettransport.py(1 hunks)ably/types/options.py(3 hunks)ably/types/presence.py(4 hunks)test/ably/realtime/presencemap_test.py(1 hunks)test/ably/realtime/realtimepresence_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- ably/realtime/connection.py
- ably/realtime/connectionmanager.py
- ably/types/options.py
- ably/transport/websockettransport.py
🧰 Additional context used
🧬 Code graph analysis (5)
test/ably/realtime/realtimepresence_test.py (4)
ably/types/channelstate.py (1)
ChannelState(8-15)ably/types/presence.py (2)
PresenceAction(24-29)action(62-63)ably/realtime/realtimepresence.py (5)
get(383-438)subscribe(457-486)enter(89-106)leave(129-145)update(108-127)ably/realtime/presencemap.py (1)
get(99-109)
ably/realtime/realtime_channel.py (4)
ably/realtime/realtimepresence.py (4)
RealtimePresence(47-768)get(383-438)set_presence(513-593)act_on_channel_state(719-744)ably/types/flags.py (2)
has_flag(18-19)Flag(4-15)ably/realtime/connection.py (2)
state(109-111)state(120-121)ably/types/presence.py (4)
action(62-63)PresenceMessage(32-237)get(253-263)from_encoded_array(233-237)
ably/types/presence.py (5)
ably/types/mixins.py (5)
EncodeDataMixin(29-130)encoding(35-36)encoding(39-43)decode(46-126)from_encoded_array(129-130)ably/types/typedbuffer.py (4)
TypedBuffer(39-104)from_obj(57-90)buffer(93-94)decode(100-104)ably/util/crypto.py (4)
CipherData(122-131)encrypt(92-98)cipher_type(118-119)encoding_str(130-131)ably/util/exceptions.py (1)
AblyException(9-84)ably/types/message.py (7)
id(77-78)id(81-82)connection_id(85-86)encrypt(100-116)data(69-70)client_id(73-74)from_encoded(181-206)
ably/realtime/realtimepresence.py (7)
ably/realtime/presencemap.py (9)
PresenceMap(62-341)get(99-109)sync_in_progress(95-97)wait_sync(317-329)start_sync(249-263)remove(159-204)put(111-157)end_sync(265-315)clear(331-341)ably/types/channelstate.py (1)
ChannelState(8-15)ably/realtime/realtime_channel.py (5)
presence(733-735)state(713-715)state(718-719)attach(164-202)get(768-793)ably/types/presence.py (6)
PresenceAction(24-29)PresenceMessage(32-237)member_key(82-84)id(58-59)action(62-63)get(253-263)ably/util/eventemitter.py (5)
EventEmitter(25-186)once_async(169-182)on(45-87)off(134-167)_emit(184-186)ably/util/exceptions.py (1)
AblyException(9-84)ably/transport/websockettransport.py (1)
ProtocolMessageAction(34-56)
ably/realtime/presencemap.py (1)
ably/types/presence.py (11)
PresenceAction(24-29)PresenceMessage(32-237)is_synthesized(90-99)timestamp(78-79)parse_id(101-123)action(62-63)id(58-59)client_id(66-67)connection_id(70-71)data(74-75)extras(87-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.14)
- GitHub Check: check (3.12)
- GitHub Check: check (3.10)
- GitHub Check: check (3.13)
- GitHub Check: check (3.8)
- GitHub Check: check (3.11)
- GitHub Check: check (3.9)
🔇 Additional comments (21)
test/ably/realtime/realtimepresence_test.py (1)
33-56: LGTM - Test fixture setup is well-structured.The parameterized test class with proper setup/teardown using pytest fixtures follows good testing practices.
ably/realtime/realtime_channel.py (5)
139-141: Lazy import to avoid circular dependency is appropriate here.The inline import of
RealtimePresenceinside__init__prevents circular imports between modules. This is a common pattern, though documenting the reason would help future maintainers.
535-555: Correct implementation of HAS_PRESENCE flag handling (RTP1).The
has_presenceflag is properly extracted from the protocol message flags and passed through state notifications.
639-640: RTP5 presence notification on state changes is correct.The presence instance is notified of channel state changes with the
has_presenceflag and any error information, enabling proper presence lifecycle management.
732-735: LGTM - Property exposes presence instance correctly.
578-590: PRESENCE and SYNC message handling is correct.The implementation properly decodes presence messages using the channel's cipher and routes them to the presence instance with appropriate
is_syncflags. Thesync_channel_serialis correctly passed for SYNC messages per RTP18. Thecipherproperty is available on RealtimeChannel and the method signatures forPresenceMessage.from_encoded_array()andset_presence()match the implementation.test/ably/realtime/presencemap_test.py (2)
1-14: LGTM - Well-organized test file with clear documentation.The imports and structure are appropriate. Tests are properly organized by functionality.
722-737: Good test for idempotent start_sync behavior.This correctly verifies that calling
start_syncmultiple times during an ongoing sync doesn't reset the residual members.ably/types/presence.py (5)
140-142: Inconsistent return value inencryptmethod.The method returns
Noneimplicitly in most paths but has an early return on line 142 that returns nothing (after the previous review,return Truewas changed). This is now consistent. The past review comment has been addressed.
90-99: LGTM -is_synthesized()correctly implements RTP2b1.The method properly identifies synthesized leave events by checking if the connectionId is an initial substring of the message id.
101-123: LGTM -parse_id()correctly implements RTP2b2 parsing.The method properly parses the id format and raises appropriate errors for invalid formats.
232-237: LGTM -from_encoded_array()provides convenient batch decoding.The implementation correctly delegates to
from_encodedfor each item.
179-180: No changes needed—the validation placement is correct.The type check on line 179 is intentionally positioned after transformations. It's a final guard to ensure that after attempting to encode various data types (dict/list to JSON, bytes/bytearray to base64), only supported types remain. This design is confirmed by existing tests (
test_unsupported_payload_must_raise_exception) which verify that unsupported types likeint,float, andboolare properly rejected with an AblyException. The validation correctly catches any unsupported types that didn't match the preceding if-elif conditions.Likely an incorrect or invalid review comment.
ably/realtime/presencemap.py (6)
15-59: LGTM -_is_newercorrectly implements RTP2b newness comparison.The function properly handles:
- RTP2b1: Synthesized messages compared by timestamp
- RTP2b1a: Equal timestamps favor incoming message
- RTP2b2: Normal messages compared by msgSerial then index
The edge cases for
Nonetimestamps are also handled correctly.
111-157: LGTM -putmethod correctly implements RTP2d.The method properly:
- Stores ENTER/UPDATE/PRESENT actions as PRESENT
- Removes members from residual during sync
- Applies newness checks before updating
159-204: LGTM -removemethod correctly implements RTP2h.The implementation correctly:
- RTP2h1: Removes members outside of sync
- RTP2h2: Marks members as ABSENT during sync
- Applies newness checks before action
317-329:wait_synccallback is executed synchronously when sync not in progress.This is correct behavior per the docstring. However, ensure callers are aware that the callback may be invoked synchronously (immediately) rather than always being deferred.
331-341: LGTM -clearproperly resets all state.The method correctly clears the map, residual members, sync state, and callbacks.
265-315: Callbacks are explicitly typed as synchronous and no async support is needed.The
callbackparameter is typed asCallable[[], None], which explicitly specifies synchronous callbacks. The actual callback used in the codebase (on_sync_complete) is synchronous—it only callsfuture.set_result(None). The type annotation prevents accidental registration of coroutines, andend_syncis invoked synchronously from message processing handlers.Likely an incorrect or invalid review comment.
ably/realtime/realtimepresence.py (2)
231-231: Good use ofcan_assume_client_idfor authorization checks.The code correctly uses
auth.can_assume_client_id(client_id)to validate whether the connection can assume the specified clientId, addressing the past review feedback.
612-618: Correct handling of sync completion whenhas_presence=False.The code properly calls
members.end_sync()(line 618) after clearing members when there's no presence on the channel, ensuring any waiters in_wait_for_sync()are unblocked. This is the correct pattern.
owenpearson
force-pushed
the
realtimepresence
branch
from
5b72275 to
253ce0b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/651/features
ttypic
left a comment
ttypic
left a comment
There was a problem hiding this comment.
Overall looks good to me!
- one coderabbit comment looks legit
- added small code-style comment about 2 imports in the middle of the code
- added 2 comments that's better to fix outside of this PR
owenpearson
force-pushed
the
realtimepresence
branch
from
253ce0b to
ffa553b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
test/ably/realtime/realtimepresence_test.py (2)
126-135: Consider using event-based synchronization instead of fixed sleeps.The tests use
asyncio.sleep()for waiting on events (e.g., lines 127, 134, 159, 164). While acceptable for integration tests, this pattern can lead to flaky tests or unnecessarily slow test runs. Consider usingasyncio.wait_for()with futures or event-based callbacks where possible, similar to the pattern already used intest_presence_enter_without_attach(lines 66-81).That said, this is a minor concern for integration tests and doesn't block the PR.
697-699: TODO noted for the 6-second sleep.The sleep is required because
remainPresentForis set to 5000ms, so waiting 6 seconds ensures the LEAVE event is received. Consider adding a brief comment explaining why 6 seconds is the minimum wait time (5s remainPresentFor + buffer).# Wait for presence events including LEAVE (which arrives after remainPresentFor timeout) - # TODO: address this sleep + # remainPresentFor is set to 5000ms, so we need to wait at least 5s for the LEAVE event + # plus some buffer for network/processing delays await asyncio.sleep(6.0)ably/types/presence.py (2)
159-162: Redundantstr()conversion afterjson.dumps().
json.dumps()already returns a string, so the subsequentdata = str(data)on line 162 is unnecessary.if isinstance(data, (dict, list)): encoding.append('json') data = json.dumps(data) - data = str(data)
179-180: Data type validation includes types that can't occur at this point.After the encoding transformations,
datacan only bestr,bytes, orNone. The check forlistanddictis unnecessary since they've already been JSON-serialized to strings. This isn't a bug, just slightly misleading.- if not (isinstance(data, (bytes, str, list, dict, bytearray)) or data is None): + if not (isinstance(data, (bytes, str)) or data is None): raise AblyException("Invalid data payload", 400, 40011)ably/realtime/presencemap.py (2)
111-157: Consider validating action in put() method.The method handles
ENTER,UPDATE, andPRESENTas documented, but doesn't validate the action. IfLEAVEorABSENTis passed, it would store them directly (line 138) without conversion toPRESENT, which may not be the intended behavior per RTP2d.Apply this diff to add validation:
def put(self, item: PresenceMessage) -> bool: """ Add or update a presence member (RTP2d). For ENTER, UPDATE, or PRESENT actions, the message is stored in the map with action set to PRESENT (if it passes the newness check). Args: item: The presence message to add/update Returns: True if the item was added/updated, False if rejected due to newness check """ + # Validate that put() is only used for ENTER/UPDATE/PRESENT + if item.action not in (PresenceAction.ENTER, PresenceAction.UPDATE, PresenceAction.PRESENT): + self._logger.warning(f"PresenceMap.put: unexpected action {item.action}, ignoring") + return False + # RTP2d: ENTER, UPDATE, PRESENT all get stored as PRESENT if item.action in (PresenceAction.ENTER, PresenceAction.UPDATE, PresenceAction.PRESENT):
159-204: Consider validating action in remove() method.The method is documented for
LEAVEaction (RTP2h) but doesn't validate thatitem.action == PresenceAction.LEAVE. If called with other actions, it would still process them, which may not be the intended behavior.Apply this diff to add validation:
def remove(self, item: PresenceMessage) -> bool: """ Remove a presence member (RTP2h). During a SYNC, the member is marked as ABSENT rather than removed. Outside of SYNC, the member is removed from the map. Args: item: The presence message with LEAVE action Returns: True if a member was removed/marked absent, False if no action taken """ + # Validate that remove() is used for LEAVE action + if item.action != PresenceAction.LEAVE: + self._logger.warning(f"PresenceMap.remove: unexpected action {item.action}, ignoring") + return False + key = self._member_key_fn(item)ably/realtime/realtimepresence.py (3)
189-273: Misleading comment on state handling.Line 269 comment mentions "DETACHED" in the else block, but
DETACHEDis already handled in theelifat line 260. The else block (lines 268-273) actually handles states likeFAILED,SUSPENDED, andDETACHING.Apply this diff to clarify the comment:
- else: - # RTP8g: DETACHED, FAILED, etc. + else: + # RTP8g: FAILED, SUSPENDED, DETACHING, etc. raise AblyException(
383-438: Accessing private __internal_state_emitter uses name mangling.Line 426 accesses
self.channel._RealtimeChannel__internal_state_emittervia Python's name mangling for private attributes. While this works, it tightly couples this code to RealtimeChannel's internal implementation and could break if the channel class changes.Consider whether RealtimeChannel should expose a public method like
wait_for_state()orwait_for_state_change()to avoid reaching into private internals.
771-785: Monkey-patching PresenceAction for action name conversion.Lines 771-785 add a
_action_namestatic method toPresenceActionvia monkey-patching. While this works, it would be more maintainable to define this method directly in thePresenceActionclass inably/types/presence.py.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
ably/realtime/presencemap.py(1 hunks)ably/realtime/realtime_channel.py(8 hunks)ably/realtime/realtimepresence.py(1 hunks)ably/transport/websockettransport.py(1 hunks)ably/types/presence.py(4 hunks)test/ably/realtime/presencemap_test.py(1 hunks)test/ably/realtime/realtimepresence_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/ably/realtime/presencemap_test.py (2)
ably/realtime/presencemap.py (10)
PresenceMap(62-351)_is_newer(15-59)put(111-157)remove(159-204)values(206-216)clear(331-351)sync_in_progress(95-97)start_sync(249-263)end_sync(265-315)wait_sync(317-329)ably/types/presence.py (13)
PresenceAction(24-29)PresenceMessage(32-237)id(58-59)connection_id(70-71)client_id(66-67)action(62-63)is_synthesized(90-99)parse_id(101-123)member_key(82-84)data(74-75)timestamp(78-79)to_encoded(147-203)from_encoded_array(233-237)
ably/realtime/presencemap.py (1)
ably/types/presence.py (6)
PresenceAction(24-29)PresenceMessage(32-237)is_synthesized(90-99)parse_id(101-123)id(58-59)connection_id(70-71)
test/ably/realtime/realtimepresence_test.py (6)
ably/types/channelstate.py (1)
ChannelState(8-15)ably/types/presence.py (2)
PresenceAction(24-29)action(62-63)ably/util/exceptions.py (1)
AblyException(9-84)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)ably/realtime/presencemap.py (1)
get(99-109)ably/realtime/realtimepresence.py (5)
get(383-438)subscribe(457-486)enter(89-106)leave(129-145)update(108-127)
ably/types/presence.py (3)
ably/types/mixins.py (5)
EncodeDataMixin(29-130)encoding(35-36)encoding(39-43)decode(46-126)from_encoded_array(129-130)ably/types/typedbuffer.py (4)
TypedBuffer(39-104)from_obj(57-90)buffer(93-94)decode(100-104)ably/util/crypto.py (4)
CipherData(122-131)encrypt(92-98)cipher_type(118-119)encoding_str(130-131)
ably/realtime/realtime_channel.py (4)
ably/types/presence.py (4)
PresenceMessage(32-237)action(62-63)get(253-263)from_encoded_array(233-237)ably/realtime/realtimepresence.py (4)
RealtimePresence(47-768)get(383-438)set_presence(513-593)act_on_channel_state(719-744)ably/types/flags.py (2)
has_flag(18-19)Flag(4-15)ably/types/channelstate.py (1)
ChannelState(8-15)
🔇 Additional comments (37)
ably/transport/websockettransport.py (1)
180-187: LGTM! Correctly routes PRESENCE and SYNC protocol messages to channel handling.The expansion of the action tuple to include
PRESENCEandSYNCalongsideMESSAGEenables the channel layer to process presence-related protocol messages, which is required for the new realtime presence functionality.test/ably/realtime/realtimepresence_test.py (1)
1-17: Comprehensive test coverage for RealtimePresence.The test suite provides excellent coverage of presence operations including:
- Basic operations (enter, leave, update)
- Implicit attach behavior
- enterClient/updateClient/leaveClient workflows
- Connection lifecycle interactions
- Auto re-entry after suspension
- SYNC behavior and presence map refresh
The tests are well-structured with clear docstrings referencing RTP spec requirements.
ably/realtime/realtime_channel.py (4)
140-142: Lazy import pattern is appropriate here.The inline import of
RealtimePresenceavoids circular import issues between the channel and presence modules. This is a common and acceptable pattern for such cases.
579-589: LGTM! Presence and SYNC message handling is correctly implemented.The implementation properly:
- Decodes presence messages using
PresenceMessage.from_encoded_arraywith the channel cipher- Distinguishes between PRESENCE (is_sync=False) and SYNC (is_sync=True) actions
- Passes the channel serial for SYNC messages to enable proper sync tracking
599-600: Channel state changes correctly propagate to presence.The updated
_notify_statemethod properly notifies the presence instance of channel state changes viaact_on_channel_state, passing both thehas_presenceflag and any error reason. This enables presence to react appropriately to channel lifecycle events (e.g., clearing state on FAILED, handling sync on ATTACHED).Also applies to: 638-639
731-734: LGTM! Public presence property provides access to RealtimePresence.The property correctly exposes the channel's presence instance, enabling
channel.presence.enter(),channel.presence.get(), etc.test/ably/realtime/presencemap_test.py (2)
491-502: Direct access to internal_mapfor testing ABSENT behavior is acceptable.The test directly manipulates
self.presence_map._map[absent.member_key] = absentto test thatvalues()excludes ABSENT members. While accessing private members is generally discouraged, this is appropriate for unit tests that need to verify internal state behavior that can't otherwise be tested through the public API.
1-14: Excellent unit test coverage for PresenceMap and PresenceMessage.The tests comprehensively cover:
- PresenceMessage helper methods (is_synthesized, parse_id, member_key, to_encoded, from_encoded_array)
- Newness comparison logic per RTP2b specification
- PresenceMap basic operations (put, remove, values, list, clear)
- SYNC workflow (start_sync, end_sync, residual handling, ABSENT member lifecycle)
- Edge cases and error conditions
Well-structured with clear test method names and docstrings.
ably/types/presence.py (4)
90-99: LGTM! is_synthesized() correctly implements RTP2b1.The method properly checks if the message is a synthesized leave event by verifying that the id doesn't start with the connection_id prefix.
101-123: LGTM! parse_id() correctly parses presence message IDs per RTP2b2.The method properly extracts msgSerial and index components from the id string with appropriate error handling for invalid formats.
125-145: LGTM! encrypt() method correctly handles data encryption.The method properly:
- Returns early if data is already encrypted (CipherData)
- Adds appropriate encoding prefixes for string and dict/list data before encryption
- Uses TypedBuffer to properly serialize the data before encryption
- Stores the result as CipherData for later wire encoding
232-237: LGTM! from_encoded_array() correctly decodes arrays of presence messages.Simple and effective implementation using list comprehension.
ably/realtime/presencemap.py (8)
15-59: LGTM! Newness comparison logic correctly implements RTP2b.The
_is_newerfunction properly handles:
- Synthesized message comparison by timestamp (RTP2b1) with correct tie-breaker logic (RTP2b1a)
- Non-synthesized message comparison by msgSerial and index (RTP2b2)
- Edge cases for None timestamps
The intentional ValueError propagation from
parse_id()is appropriate, as documented in the function's docstring.
72-92: LGTM! Clean initialization with sensible defaults.The constructor properly initializes all internal state with appropriate defaults for optional parameters.
206-216: LGTM! Correctly filters ABSENT members.
218-247: LGTM! Filtering logic correctly implements RTP11.
249-263: LGTM! Correctly handles multiple start_sync calls during ongoing SYNC.
265-315: LGTM! Correctly handles SYNC completion with proper callback invocation.The method properly:
- Collects and removes ABSENT members
- Handles residual members with defensive key checks
- Invokes all registered callbacks with per-callback exception handling
- Cleans up state
317-329: LGTM! Correctly queues or immediately invokes callback based on sync state.
331-351: LGTM! Properly invokes callbacks before clearing to prevent hanging Futures.The implementation addresses the requirement to resolve waiting Futures when clearing state, ensuring callers aren't left blocked.
ably/realtime/realtimepresence.py (17)
28-44: LGTM! Helper functions correctly handle clientId resolution and anonymous detection.The logic appropriately falls back when not connected and correctly identifies anonymous or wildcard clients.
62-87: LGTM! Clean initialization of presence maps and internal state.
89-106: LGTM! Correctly validates clientId and delegates to internal handler.
108-127: LGTM! Update method correctly mirrors enter logic.
129-145: LGTM! Leave method correctly validates and delegates.
147-187: LGTM! Client-scoped methods correctly delegate to internal handlers.
350-381: LGTM! Clean message sending and queuing with Future-based async coordination.
440-455: LGTM! Clean sync waiting implementation using Future and callback.
457-511: LGTM! Subscribe and unsubscribe correctly implement implicit attach and delegate to internal EventEmitter.
513-593: LGTM! Correctly processes incoming presence with proper SYNC handling.The implementation correctly:
- Manages SYNC lifecycle with cursor tracking
- Updates main and internal presence maps
- Synthesizes LEAVE events for residual/absent members
- Uses
datetime.now(timezone.utc)(not deprecatedutcnow())
595-624: LGTM! Correctly handles ATTACHED state with proper presence sync initialization.The method appropriately:
- Starts SYNC when presence is available
- Synthesizes leaves and clears when no presence
- Re-enters own members
- Sends pending presence messages
626-669: LGTM! Re-entry logic correctly implements RTP17g.The implementation properly:
- Suppresses message ID when connection changes (RTP17g1)
- Re-enters each member asynchronously
- Handles errors with logging
Note: Lines 667-669 mention emitting update events with errors is "not implemented in this phase" - this is acknowledged as a future enhancement.
671-699: LGTM! Correctly sends pending presence and manages Future resolution.
701-717: LGTM! Correctly synthesizes LEAVE events with proper timestamp handling.Uses
datetime.now(timezone.utc)appropriately.
719-744: LGTM! Correctly handles channel state transitions with proper cleanup.The method appropriately:
- Delegates to on_attached for ATTACHED state
- Clears maps and fails pending for DETACHED/FAILED
- Resets sync state for SUSPENDED
746-768: LGTM! Correctly fails pending presence messages with proper Future rejection.
275-348: The code's channel state validation does not align with RTP10e requirements.RTP10e specifies: "If the client is not currently ENTERED then an exception is raised." This requires checking the presence entry state (whether the client has previously entered the presence set), not just the channel state. A client must have previously entered the presence set before they can leave it.
The current code at lines 288-298 validates channel state (CONNECTING/CONNECTED/DISCONNECTED for connection, and ATTACHED/ATTACHING/INITIALIZED/FAILED for channel), but does not validate whether the client has actually ENTERED the presence set. The channel state checks may be appropriate for operational purposes, but they don't fulfill RTP10e's requirement to verify presence entry state before allowing a leave operation.
owenpearson
force-pushed
the
realtimepresence
branch
from
ffa553b to
affb25f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ably/types/presence.py (1)
159-162: Redundantstr()call afterjson.dumps().
json.dumps()already returns a string, so the subsequentdata = str(data)on line 162 is unnecessary.if isinstance(data, (dict, list)): encoding.append('json') data = json.dumps(data) - data = str(data)test/ably/realtime/realtimepresence_test.py (1)
127-135: Consider event-based waiting instead of fixed sleeps.Using
asyncio.sleep(0.5)for synchronization can lead to flaky tests. Consider waiting for specific events or using a more deterministic approach.For example, you could wait for the presence event to be received:
# Instead of: await asyncio.sleep(0.5) assert (PresenceAction.ENTER, 'client2') in events # Consider: await asyncio.wait_for( wait_for_event(lambda: (PresenceAction.ENTER, 'client2') in events), timeout=5.0 )This is a minor concern as the current implementation should work in most cases.
ably/realtime/realtimepresence.py (2)
424-431: Access to private attribute may be brittle.Line 426 accesses
self.channel._RealtimeChannel__internal_state_emitterusing Python's name-mangling convention. While this may be intentional for internal communication between tightly coupled classes, it creates a brittle dependency. IfRealtimeChannelrenames or removes this attribute, this code will break.Consider either:
- Making
__internal_state_emittera protected attribute (_internal_state_emitter) inRealtimeChannelif it's meant for internal package use- Exposing a public method on
RealtimeChannelfor waiting on state changes
770-785: Consider defining_action_namein thePresenceActionclass definition.While the monkey-patching approach works, defining
_action_namedirectly in thePresenceActionclass inably/types/presence.pywould be cleaner and avoid import-order dependencies. This would also make the helper more discoverable and type-checker friendly.If
PresenceActionis used across multiple modules that need human-readable action names, the helper belongs with its definition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
ably/realtime/presencemap.py(1 hunks)ably/realtime/realtime_channel.py(8 hunks)ably/realtime/realtimepresence.py(1 hunks)ably/transport/websockettransport.py(1 hunks)ably/types/presence.py(4 hunks)test/ably/realtime/presencemap_test.py(1 hunks)test/ably/realtime/realtimepresence_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ably/realtime/presencemap_test.py
🧰 Additional context used
🧬 Code graph analysis (4)
test/ably/realtime/realtimepresence_test.py (8)
ably/realtime/realtime.py (1)
connection(132-134)ably/realtime/realtime_channel.py (5)
presence(732-734)get(767-792)state(712-714)state(717-718)attach(165-203)ably/types/presence.py (2)
PresenceAction(24-29)action(62-63)test/ably/testapp.py (2)
get_test_vars(41-70)get_ably_realtime(80-83)ably/realtime/connectionmanager.py (2)
request_state(469-492)notify_state(567-613)ably/realtime/presencemap.py (1)
get(99-109)ably/realtime/realtimepresence.py (1)
get(383-438)ably/types/channelsubscription.py (1)
channel(19-20)
ably/types/presence.py (4)
ably/types/mixins.py (5)
EncodeDataMixin(29-130)encoding(35-36)encoding(39-43)decode(46-126)from_encoded_array(129-130)ably/types/typedbuffer.py (4)
TypedBuffer(39-104)from_obj(57-90)buffer(93-94)decode(100-104)ably/util/crypto.py (3)
CipherData(122-131)cipher_type(118-119)encoding_str(130-131)ably/util/exceptions.py (1)
AblyException(9-84)
ably/realtime/presencemap.py (2)
ably/types/presence.py (8)
PresenceAction(24-29)PresenceMessage(32-237)is_synthesized(90-99)parse_id(101-123)get(253-263)id(58-59)client_id(66-67)connection_id(70-71)ably/realtime/realtimepresence.py (1)
get(383-438)
ably/realtime/realtimepresence.py (5)
ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/realtime/presencemap.py (10)
PresenceMap(62-351)get(99-109)sync_in_progress(95-97)wait_sync(317-329)start_sync(249-263)remove(159-204)put(111-157)end_sync(265-315)values(206-216)clear(331-351)ably/types/channelstate.py (1)
ChannelState(8-15)ably/types/presence.py (13)
PresenceAction(24-29)PresenceMessage(32-237)client_id(66-67)member_key(82-84)data(74-75)id(58-59)action(62-63)encrypt(125-145)to_encoded(147-203)get(253-263)connection_id(70-71)is_synthesized(90-99)timestamp(78-79)ably/util/eventemitter.py (5)
EventEmitter(25-186)once_async(169-182)on(45-87)off(134-167)_emit(184-186)
🔇 Additional comments (34)
ably/transport/websockettransport.py (1)
180-187: LGTM! Correct routing of PRESENCE and SYNC protocol messages.The addition of
PRESENCEandSYNCactions to the channel message routing is necessary for presence functionality. These protocol messages will now be correctly forwarded to channel handlers for processing.ably/realtime/realtime_channel.py (6)
140-142: Inline import is acceptable to avoid circular dependency.The inline import of
RealtimePresencewithin__init__is a common Python pattern to handle circular imports betweenrealtime_channel.pyandrealtimepresence.py.
536-556: LGTM! Correct implementation of RTP1 HAS_PRESENCE flag handling.The
has_presenceflag is properly extracted from the ATTACHED message flags and propagated through the state transition to enable presence sync behavior.
579-589: LGTM! Correct handling of PRESENCE and SYNC protocol messages.The differentiation between
PRESENCE(live events withis_sync=False) andSYNC(initial state withis_sync=True) is correct per RTP18. Thesync_channel_serialis properly extracted and passed for sync cursor tracking.Consider whether error handling should be added for
from_encoded_arrayfailures, similar to the MESSAGE handling at lines 572-576 which catches decode errors. Currently, a decode failure would propagate as an unhandled exception.
599-600: LGTM! Backward-compatible signature extension.The
has_presenceparameter defaults toFalse, maintaining compatibility with existing callers while enabling presence-aware state transitions.
638-639: LGTM! Correct RTP5 implementation.Presence is properly notified of channel state changes after the transition completes, allowing it to handle sync completion, member clearing, or auto-reentry as needed.
731-734: LGTM! Standard property accessor for presence.ably/types/presence.py (4)
90-123: LGTM! Correct RTP2b1/RTP2b2 implementation.
is_synthesized()correctly identifies synthesized leave events by checking the id prefix pattern.parse_id()properly extracts msgSerial and index with appropriate error handling for malformed IDs.
125-145: LGTM! Encryption logic is correct.The method properly handles different data types, appends appropriate encoding markers, and encrypts using the provided cipher. The early return for
Nonebuffer is consistent with the base Message pattern.
179-203: LGTM! Result building is correct.The validation correctly catches unsupported data types (e.g., raw integers, floats) before serialization. The result dictionary properly uses camelCase keys for wire protocol compatibility.
232-237: LGTM! Clean array decoding implementation.test/ably/realtime/realtimepresence_test.py (5)
20-30: LGTM! Well-structured helper for simulating connection suspension.The helper correctly sequences through DISCONNECTED to SUSPENDED states, enabling reliable testing of suspension/reconnection scenarios.
167-183: LGTM! Proper error case testing with cleanup.The test correctly verifies that anonymous clients receive an appropriate error when attempting to enter presence, with proper resource cleanup in the
finallyblock.
374-393: LGTM! Good coverage of multi-client presence operations.The test properly verifies that multiple clients can be entered on a single connection using wildcard authentication.
630-723: LGTM! Thorough testing of auto-reentry with connection ID changes.The test properly verifies RTP17g requirements by:
- Tracking connection IDs before and after suspension
- Verifying LEAVE events for old connection
- Verifying ENTER events for new connection
- Checking that message IDs differ
805-886: LGTM! Comprehensive testing of presence preservation during suspension.The test correctly verifies that:
- Presence map is preserved when connection goes to SUSPENDED
- Only changed members trigger events after reconnection
- Final state correctly reflects changes that occurred during suspension
ably/realtime/presencemap.py (5)
15-59: LGTM! Correct RTP2b newness comparison implementation.The function correctly handles:
- RTP2b1: Synthesized messages compared by timestamp
- RTP2b1a: Equal timestamps favor the incoming message
- RTP2b2: Non-synthesized messages compared by msgSerial, then index
111-157: LGTM! Correct RTP2d implementation for storing presence members.The method properly:
- Converts ENTER/UPDATE/PRESENT actions to PRESENT for consistent storage
- Tracks residual members during SYNC operations
- Applies newness checks to prevent stale updates
159-204: LGTM! Correct RTP2h implementation for member removal.The method properly handles:
- RTP2h1: Direct removal outside of SYNC
- RTP2h2: Marking as ABSENT during SYNC for deferred processing
- Newness validation before any action
249-315: LGTM! Correct RTP18/RTP19 SYNC lifecycle implementation.
start_sync()properly captures residual members for tracking, andend_sync()correctly:
- Removes ABSENT members (RTP2h2b)
- Returns residual members for synthesized LEAVE emission (RTP19)
- Notifies waiting callbacks after completion
331-351: LGTM! Proper cleanup with callback notification.The method correctly notifies pending sync callbacks before clearing state, ensuring that any Futures waiting on sync completion are resolved and callers aren't left blocked indefinitely.
ably/realtime/realtimepresence.py (13)
1-26: LGTM!The module structure, imports, and logger setup are well-organized. The use of
TYPE_CHECKINGcorrectly handles the circular dependency withRealtimeChannel.
28-45: LGTM!The helper functions correctly handle client ID resolution and anonymous/wildcard detection. The check for connection state in
_is_anonymous_or_wildcardis appropriate.
47-87: LGTM!The constructor properly initializes all required state including the dual presence maps (main and own-members), subscriptions emitter, and pending presence queue.
89-187: LGTM!The public presence methods correctly validate client ID requirements and delegate to internal implementation methods. The exception handling follows the corrected format.
189-273: LGTM!The internal enter/update logic correctly validates connection state, checks client ID permissions using
can_assume_client_id, and routes messages based on channel state. The implicit attach logic properly queues messages while attachment is in progress.
275-348: LGTM!The leave logic correctly avoids implicitly attaching the channel (RTP10e), which is the appropriate behavior for leave operations. State validation and message handling are correct.
350-381: LGTM!The send and queue mechanisms are well-designed. The use of futures in
_queue_presenceallows callers to await the result of queued operations.
457-511: LGTM!The subscription methods correctly trigger implicit channel attachment and properly delegate to the internal EventEmitter. Argument parsing handles multiple call signatures appropriately.
513-593: LGTM!The presence message processing logic correctly handles SYNC operations, maintains both the main and own-members maps, and synthesizes leave events for residual/absent members. The use of
datetime.now(timezone.utc)is correct.
595-669: LGTM!The attachment handling correctly manages both sync and no-sync scenarios, and the automatic re-entry logic properly suppresses message IDs when the connection has changed. The fire-and-forget task creation for re-entry is appropriate for this use case.
671-699: LGTM!The pending presence message handling is robust. The code correctly atomically clears the queue, sends all messages, and resolves/rejects all associated futures based on the outcome.
701-717: LGTM!The synthesized leave event generation is correctly implemented with timezone-aware timestamps and proper event emission.
746-768: LGTM!The pending presence failure handling correctly atomically clears the queue and rejects all associated futures with an appropriate exception.
owenpearson
force-pushed
the
realtimepresence
branch
from
affb25f to
a2d7a6d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/651/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
ably/types/presence.py (1)
159-162: Redundantstr()call afterjson.dumps().
json.dumps()already returns a string, making the subsequentstr(data)call on line 162 unnecessary.if isinstance(data, (dict, list)): encoding.append('json') data = json.dumps(data) - data = str(data)test/ably/realtime/realtimepresence_test.py (2)
127-135: Consider event-based waiting for more robust tests.The test uses
asyncio.sleep(0.5)to wait for presence events. While functional, this could be flaky in slower environments. Consider using event-based waiting (e.g., waiting for the specific event count) as a more robust alternative.
556-564: Redundant close call after testing closed connection.Line 564 calls
await client.close()on an already-closed client. While harmless, it's unnecessary since the connection was already closed on line 554.# Try to enter - should fail try: await channel.presence.enter_client('client1', 'data') pytest.fail('Should have raised exception for closed connection') except Exception as e: # Should get an error about closed/failed connection assert 'closed' in str(e).lower() or 'failed' in str(e).lower() or '80017' in str(e) - - await client.close()ably/realtime/realtimepresence.py (4)
189-273: Consider error handling for fire-and-forget attach.Line 262 creates a fire-and-forget task for
channel.attach(). If the attach fails, the exception will only be visible as a warning in asyncio's task exception handler, not in application logs.Consider wrapping the task creation with error logging:
async def _attach_with_logging(): try: await channel.attach() except Exception as e: log.error(f'RealtimePresence: implicit attach failed: {e}') asyncio.create_task(_attach_with_logging())Alternatively, store the task reference and optionally await or monitor it later:
# Store task for potential monitoring attach_task = asyncio.create_task(channel.attach()) # Optionally add done callback for error logging attach_task.add_done_callback(lambda t: t.exception() and log.error(f'Attach failed: {t.exception()}'))The same pattern applies to line 473 in the
subscribemethod.
383-438: Consider providing a public API for waiting on channel state changes.Line 426 accesses the private
__internal_state_emitterattribute fromRealtimeChannel. While this works, direct access to private/internal members creates tight coupling and may break if the internal implementation changes.Consider one of these approaches:
- Add a public method on
RealtimeChannellikewait_for_state_change()orwhen_state(state)to encapsulate this pattern.- Use the public
channel.once_async()if it provides equivalent functionality.- If
__internal_state_emitteris intended for internal package use, document this cross-component dependency.Example public API:
# In RealtimeChannel async def wait_for_state_change(self) -> ChannelStateChange: """Wait for the next channel state change.""" return await self.__internal_state_emitter.once_async()Then use it here:
state_change = await self.channel.wait_for_state_change()
626-644: Prefer public API over direct access to internal PresenceMap state.Line 632 directly accesses
_my_members._map, which is a private attribute ofPresenceMap. This creates coupling to the internal implementation and could break if PresenceMap's internal structure changes.Consider either:
- Adding a public method on
PresenceMapto iterate over entries (e.g.,items(),values_with_metadata()).- If this access pattern is intentional for tight integration within the same package, document this internal dependency.
Example public API on PresenceMap:
# In PresenceMap def items(self): """Iterate over (key, message) pairs.""" return self._map.items()Then use it here:
for _client_id, entry in list(self._my_members.items()): # ...
771-785: Consider alternatives to monkey-patching for utility methods.Monkey-patching
PresenceAction._action_nameworks but modifies a type defined elsewhere, which can make the codebase harder to reason about (the method appears onPresenceActionbut is defined here).Consider these alternatives:
Define as a standalone utility function (no monkey-patch):
def presence_action_name(action: int) -> str: """Convert presence action to string name.""" names = { ... } return names.get(action, f'unknown({action})')If PresenceAction is defined in this package, add the method directly to the class definition.
If monkey-patching is the preferred pattern for extending external types in this codebase, this is acceptable. Just ensure it's documented or consistent with the project's conventions.
The current approach is functional, so this is purely a stylistic consideration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
ably/realtime/connection.py(3 hunks)ably/realtime/connectionmanager.py(3 hunks)ably/realtime/presencemap.py(1 hunks)ably/realtime/realtime_channel.py(8 hunks)ably/realtime/realtimepresence.py(1 hunks)ably/transport/websockettransport.py(1 hunks)ably/types/options.py(3 hunks)ably/types/presence.py(4 hunks)test/ably/realtime/presencemap_test.py(1 hunks)test/ably/realtime/realtimepresence_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ably/types/options.py
- ably/realtime/connection.py
🧰 Additional context used
🧬 Code graph analysis (6)
test/ably/realtime/realtimepresence_test.py (3)
ably/types/presence.py (1)
PresenceAction(24-29)ably/realtime/presencemap.py (1)
get(99-109)ably/realtime/realtimepresence.py (5)
get(383-438)subscribe(457-486)enter(89-106)leave(129-145)update(108-127)
ably/realtime/realtime_channel.py (2)
ably/types/presence.py (4)
PresenceMessage(32-237)action(62-63)get(253-263)from_encoded_array(233-237)ably/realtime/realtimepresence.py (4)
RealtimePresence(47-768)get(383-438)set_presence(513-593)act_on_channel_state(719-744)
test/ably/realtime/presencemap_test.py (2)
ably/realtime/presencemap.py (11)
PresenceMap(62-351)_is_newer(15-59)put(111-157)get(99-109)remove(159-204)values(206-216)clear(331-351)sync_in_progress(95-97)start_sync(249-263)end_sync(265-315)wait_sync(317-329)ably/types/presence.py (14)
PresenceAction(24-29)PresenceMessage(32-237)id(58-59)connection_id(70-71)client_id(66-67)action(62-63)is_synthesized(90-99)parse_id(101-123)member_key(82-84)data(74-75)timestamp(78-79)to_encoded(147-203)from_encoded_array(233-237)get(253-263)
ably/realtime/realtimepresence.py (7)
ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/realtime/presencemap.py (9)
PresenceMap(62-351)sync_in_progress(95-97)wait_sync(317-329)start_sync(249-263)remove(159-204)put(111-157)end_sync(265-315)values(206-216)clear(331-351)ably/types/channelstate.py (1)
ChannelState(8-15)ably/realtime/realtime_channel.py (8)
presence(734-736)RealtimeChannel(91-754)state(714-716)state(719-720)name(708-710)cipher(46-48)subscribe(284-336)unsubscribe(339-390)ably/types/presence.py (12)
PresenceAction(24-29)PresenceMessage(32-237)client_id(66-67)member_key(82-84)data(74-75)id(58-59)action(62-63)encrypt(125-145)to_encoded(147-203)connection_id(70-71)is_synthesized(90-99)timestamp(78-79)ably/util/eventemitter.py (5)
EventEmitter(25-186)once_async(169-182)on(45-87)off(134-167)_emit(184-186)ably/util/exceptions.py (1)
AblyException(9-84)
ably/types/presence.py (4)
ably/types/mixins.py (5)
EncodeDataMixin(29-130)encoding(35-36)encoding(39-43)decode(46-126)from_encoded_array(129-130)ably/types/typedbuffer.py (4)
TypedBuffer(39-104)from_obj(57-90)buffer(93-94)decode(100-104)ably/util/crypto.py (4)
CipherData(122-131)encrypt(92-98)cipher_type(118-119)encoding_str(130-131)ably/util/exceptions.py (1)
AblyException(9-84)
ably/realtime/connectionmanager.py (5)
ably/realtime/connection.py (3)
state(109-111)state(120-121)close(61-68)ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/types/options.py (1)
transport_params(287-288)ably/transport/websockettransport.py (2)
close(244-245)dispose(230-242)ably/realtime/realtime.py (1)
close(120-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.10)
- GitHub Check: check (3.14)
- GitHub Check: check (3.11)
- GitHub Check: check (3.8)
- GitHub Check: check (3.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.9)
🔇 Additional comments (31)
ably/transport/websockettransport.py (1)
183-190: LGTM!The routing of
PRESENCEandSYNCactions toon_channel_messageis correct and aligns with the new presence handling infrastructure inRealtimeChannel. This enables the channel to process incoming presence messages and SYNC operations per RTP specifications.ably/types/presence.py (5)
46-47: LGTM!Correctly propagates encoding to the
EncodeDataMixinbase class, enabling proper encoding state management for presence messages.
90-99: LGTM!The
is_synthesized()method correctly implements RTP2b1 by detecting synthesized leave events where theconnectionIdis not an initial substring of the messageid.
101-123: LGTM!The
parse_id()method correctly parses the presence message ID format for RTP2b2 comparison, with appropriate error handling for invalid formats.
125-145: LGTM!The
encrypt()method correctly handles encoding for different data types before encryption, matching theMessage.encrypt()pattern referenced in the docstring.
232-237: LGTM!The
from_encoded_array()method provides a clean convenience wrapper for decoding presence message arrays, consistent with the pattern inEncodeDataMixin.ably/realtime/presencemap.py (5)
15-59: LGTM!The
_is_newer()function correctly implements RTP2b newness comparison:
- RTP2b1: Synthesized messages compared by timestamp
- RTP2b1a: Equal timestamps favor the incoming message
- RTP2b2: Normal messages compared by msgSerial then index
72-109: LGTM!The
PresenceMapinitialization and basic accessors are well-designed with appropriate dependency injection for testing flexibility (custommember_key_fn,is_newer_fn, and logger).
111-157: LGTM!The
put()method correctly implements RTP2d by normalizingENTER,UPDATE, andPRESENTactions toPRESENTfor storage, while properly handling newness checks and residual member tracking during SYNC operations.
159-204: LGTM!The
remove()method correctly implements RTP2h, distinguishing between sync and non-sync scenarios: marking members asABSENTduring SYNC (RTP2h2) and removing them directly outside of SYNC (RTP2h1).
249-351: LGTM!The SYNC operations (
start_sync,end_sync,wait_sync,clear) correctly implement RTP18/RTP19 requirements:
- Residual member tracking for synthesized LEAVE events
- ABSENT member cleanup on sync completion
- Callback handling to prevent blocked Futures on channel state transitions
ably/realtime/connectionmanager.py (3)
138-148: LGTM!The RTN16d implementation correctly clears connection state (
__connection_details,connection_id,__connection_key,msg_serial) when entering SUSPENDED or terminal states (CLOSED, FAILED). The use ofconnection_id(not__connection_id) addresses the previous review feedback.
171-174: Verify intentional override behavior for transport params.The
params.update(self.options.transport_params)call allows user-provided transport params to override protocol-set values likevandresume. This enables custom params (e.g.,remainPresentForused in tests), but confirm this override capability is intended rather than accidentally allowing users to break protocol version or resume behavior.
183-199: Partial spec compliance for connection close.The change improves the close flow by sending the CLOSE protocol message before disposing the transport. However, per RTN12a, the implementation should wait for the CLOSED protocol message response before disposal. The past review discussion acknowledged keeping full compliance out of scope for this PR.
test/ably/realtime/realtimepresence_test.py (4)
20-31: LGTM!The
force_suspendedhelper correctly usesConnectionStateenum values (addressing past review feedback) and properly simulates connection suspension for testing presence auto-reentry scenarios.
33-56: LGTM!Well-structured test class with proper fixture setup, parameterization for protocol testing, and cleanup handling.
578-628: LGTM!Comprehensive test for RTP5f/RTP17 auto re-entry behavior after connection suspension. The test properly verifies that presence members are automatically re-entered after reconnection.
805-886: LGTM!The
test_suspended_preserves_presencetest thoroughly validates RTP5f/RTP11d requirements for presence map preservation during SUSPENDED state and proper synchronization after reconnection.ably/realtime/realtime_channel.py (4)
140-142: Inline import to avoid circular dependency.The inline import of
RealtimePresenceis necessary to avoid circular import issues sincerealtimepresence.pyimports from this module. This is a standard pattern for handling such dependencies.
581-591: LGTM!The PRESENCE and SYNC action handling correctly decodes presence messages and routes them to the presence layer with appropriate flags (
is_sync,sync_channel_serial) for differentiating live updates from synchronization operations per RTP18.
601-602: LGTM!The updated
_notify_statesignature and presence lifecycle integration correctly propagates thehas_presenceflag and notifies the presence layer of channel state changes per RTP5.Also applies to: 640-641
733-736: LGTM!The
presenceproperty provides clean access to the channel'sRealtimePresenceinstance, completing the public API for presence operations.test/ably/realtime/presencemap_test.py (5)
1-14: LGTM!Clean imports with all necessary dependencies for comprehensive unit testing of the PresenceMap implementation.
16-237: LGTM!Comprehensive test coverage for
PresenceMessagehelper methods including edge cases (missing IDs, invalid formats) and various data types (string, dict, list, binary). These tests validate the RTP2b support methods thoroughly.
239-332: LGTM!The newness comparison tests thoroughly validate RTP2b requirements including synthesized message handling (RTP2b1), equal timestamp behavior (RTP2b1a), and normal message comparison by msgSerial/index (RTP2b2).
335-567: LGTM!Comprehensive test coverage for PresenceMap basic operations including action normalization (RTP2d), newness checks (RTP2a), ABSENT member handling, and filtering (RTP11c).
570-772: LGTM!Excellent test coverage for SYNC operations (RTP18/RTP19) including residual member tracking, ABSENT lifecycle, callback handling, and idempotent start_sync behavior. The
test_clear_invokes_sync_callbackstest is particularly valuable for ensuring Futures don't hang on channel state transitions.ably/realtime/realtimepresence.py (4)
1-26: LGTM!The imports are well-organized, and the use of
timezonefromdatetimecorrectly addresses Python 3.12+ compatibility. The TYPE_CHECKING guard for forward references is appropriate.
28-44: LGTM!The helper functions correctly implement clientId retrieval and anonymous/wildcard detection. The conservative approach in
_is_anonymous_or_wildcard(returningFalsewhen not CONNECTED) is appropriate for safety.
47-187: LGTM!The class initialization and public API methods are well-structured. The dual PresenceMap pattern (public
membersand internal_my_members) correctly implements RTP2 and RTP17 semantics. ClientId validation is appropriate, and error handling uses the correct exception format.
671-768: LGTM!The pending message handling and state change reactions are correctly implemented. The differentiation between terminal states (DETACHED/FAILED clearing all state) and SUSPENDED (preserving members but invalidating sync) aligns with the specification. The fix for notifying sync callbacks via
members.clear()addresses the earlier review concern about hanging waiters.
owenpearson
force-pushed
the
realtimepresence
branch
from
a2d7a6d to
e1779bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ably/realtime/realtimepresence.py (1)
776-790: Consider moving _action_name to PresenceAction class definition.The monkey-patching approach works but is not ideal for maintainability. Consider adding this helper method directly to the PresenceAction class in ably/types/presence.py for cleaner code organization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
ably/realtime/presencemap.py(1 hunks)ably/realtime/realtime_channel.py(8 hunks)ably/realtime/realtimepresence.py(1 hunks)ably/transport/websockettransport.py(1 hunks)ably/types/presence.py(4 hunks)test/ably/realtime/presencemap_test.py(1 hunks)test/ably/realtime/realtimepresence_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/transport/websockettransport.py
🧰 Additional context used
🧬 Code graph analysis (5)
test/ably/realtime/presencemap_test.py (2)
ably/realtime/presencemap.py (10)
PresenceMap(62-351)_is_newer(15-59)put(111-157)get(99-109)remove(159-204)values(206-216)clear(331-351)sync_in_progress(95-97)start_sync(249-263)end_sync(265-315)ably/types/presence.py (14)
PresenceAction(24-29)PresenceMessage(32-237)id(58-59)connection_id(70-71)client_id(66-67)action(62-63)is_synthesized(90-99)parse_id(101-123)member_key(82-84)data(74-75)timestamp(78-79)to_encoded(147-203)from_encoded_array(233-237)get(253-263)
ably/realtime/presencemap.py (1)
ably/types/presence.py (7)
PresenceAction(24-29)PresenceMessage(32-237)is_synthesized(90-99)parse_id(101-123)action(62-63)id(58-59)connection_id(70-71)
ably/realtime/realtimepresence.py (7)
ably/realtime/presencemap.py (9)
PresenceMap(62-351)get(99-109)wait_sync(317-329)start_sync(249-263)remove(159-204)put(111-157)end_sync(265-315)values(206-216)clear(331-351)ably/types/channelstate.py (1)
ChannelState(8-15)ably/realtime/realtime_channel.py (7)
presence(734-736)RealtimeChannel(91-754)state(714-716)state(719-720)name(708-710)cipher(46-48)get(769-794)ably/types/presence.py (13)
PresenceAction(24-29)PresenceMessage(32-237)client_id(66-67)member_key(82-84)data(74-75)id(58-59)action(62-63)encrypt(125-145)to_encoded(147-203)get(253-263)connection_id(70-71)is_synthesized(90-99)timestamp(78-79)ably/util/eventemitter.py (5)
EventEmitter(25-186)once_async(169-182)on(45-87)off(134-167)_emit(184-186)ably/rest/auth.py (1)
can_assume_client_id(357-365)ably/transport/websockettransport.py (1)
ProtocolMessageAction(36-58)
test/ably/realtime/realtimepresence_test.py (5)
ably/types/channelstate.py (1)
ChannelState(8-15)ably/types/presence.py (2)
PresenceAction(24-29)action(62-63)ably/realtime/connectionmanager.py (1)
request_state(485-508)ably/realtime/presencemap.py (1)
get(99-109)ably/realtime/realtimepresence.py (5)
get(383-438)subscribe(457-486)enter(89-106)leave(129-145)update(108-127)
ably/types/presence.py (4)
ably/types/mixins.py (5)
EncodeDataMixin(29-130)encoding(35-36)encoding(39-43)decode(46-126)from_encoded_array(129-130)ably/types/typedbuffer.py (4)
TypedBuffer(39-104)from_obj(57-90)buffer(93-94)decode(100-104)ably/util/crypto.py (4)
CipherData(122-131)encrypt(92-98)cipher_type(118-119)encoding_str(130-131)ably/util/exceptions.py (1)
AblyException(9-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.10)
- GitHub Check: check (3.11)
- GitHub Check: check (3.8)
- GitHub Check: check (3.14)
- GitHub Check: check (3.12)
- GitHub Check: check (3.9)
- GitHub Check: check (3.13)
🔇 Additional comments (30)
ably/types/presence.py (7)
1-10: LGTM: Necessary imports for encoding/encryption features.The new imports support the encoding, encryption, and error handling functionality added to PresenceMessage.
46-46: LGTM: Correct initialization of EncodeDataMixin.Properly delegates encoding initialization to the parent mixin class.
90-99: LGTM: Correct implementation of synthesized message detection.Properly handles edge cases with None checks and correctly identifies synthesized messages per RTP2b1.
101-123: LGTM: Robust id parsing with proper error handling.Good use of try/except to catch both ValueError and IndexError, with descriptive error messages for debugging.
125-145: LGTM: Proper encryption implementation.Correctly handles different data types, updates encoding array, and performs in-place encryption. The early return pattern is appropriate for cases where no encryption is needed.
147-203: LGTM: Comprehensive wire format encoding.Properly handles all data types, encoding modes (binary/text), encryption, and optional fields. Good validation ensures only valid types are serialized.
232-237: LGTM: Simple and correct array decoding.Consistent with the pattern used in the base EncodeDataMixin class for decoding arrays.
test/ably/realtime/realtimepresence_test.py (2)
20-30: LGTM: Useful helper for testing suspended state.The helper correctly uses ConnectionState enums and provides a clean way to simulate suspension scenarios in tests.
33-886: LGTM: Comprehensive and well-structured integration tests.The test suite covers all major presence functionality including:
- Basic operations (enter/leave/update)
- Presence retrieval with sync semantics
- Subscription patterns
- Multi-client scenarios
- Connection lifecycle interactions
- Auto-reentry after suspension
- SYNC behavior and state management
Tests are well-organized with proper fixtures and good documentation.
ably/realtime/realtime_channel.py (5)
15-15: LGTM: Necessary import for presence handling.
140-142: LGTM: Proper initialization with circular dependency mitigation.Local import of RealtimePresence avoids circular dependency issues while keeping the initialization clean.
581-591: LGTM: Clean presence and sync message handling.Properly distinguishes between PRESENCE and SYNC messages, handles decryption, and delegates to the RealtimePresence instance with appropriate parameters.
601-602: LGTM: Proper presence state coordination.The channel state changes are correctly propagated to the presence instance with all necessary context (state, has_presence flag, and error information).
Also applies to: 640-641
733-736: LGTM: Standard property accessor for presence.test/ably/realtime/presencemap_test.py (1)
1-772: LGTM: Comprehensive unit test coverage for PresenceMap.Excellent test suite covering:
- PresenceMessage helpers (is_synthesized, parse_id, member_key, encoding)
- Newness comparison logic (RTP2b)
- PresenceMap basic operations (put, remove, get, list, clear)
- SYNC lifecycle (start_sync, end_sync, residual handling, ABSENT states)
- Edge cases and error conditions
Tests are well-documented and validate spec compliance (RTP2, RTP18, RTP19).
ably/realtime/realtimepresence.py (9)
28-44: LGTM: Well-designed helper functions.Good defensive logic with proper fallback for client_id retrieval and appropriate state checking in _is_anonymous_or_wildcard.
62-88: LGTM: Proper initialization of presence state.Well-structured initialization with two separate presence maps (one for all members keyed by member_key, one for own members keyed by client_id) to support auto-reentry requirements (RTP17).
89-145: LGTM: Clean public API with proper validation.Good separation between public methods and internal implementation. Proper validation for anonymous clients with clear error messages.
189-273: LGTM: Robust enter/update implementation.Comprehensive state validation, proper clientId checking using auth.can_assume_client_id, and clear logic for handling different channel states. Good encryption integration.
383-438: LGTM: Comprehensive get() implementation with proper state handling.Good handling of all channel states per RTP11 requirements. The sync waiting logic correctly uses the PresenceMap callback mechanism to avoid hanging.
457-511: LGTM: Clean subscription management.Proper implicit attach behavior on subscribe and clean delegation to EventEmitter for actual subscription management.
513-594: LGTM: Comprehensive presence message processing.Well-structured handling of incoming presence messages with proper SYNC lifecycle management, residual member handling, and synthesized LEAVE event generation per RTP19.
595-675: LGTM: Solid auto-reentry implementation.Proper implementation of RTP17 auto-reentry requirements with correct handling of connection ID changes (RTP17g1) and appropriate error handling that emits channel update events.
724-749: LGTM: Appropriate channel state handling.Correctly distinguishes between terminal states (DETACHED/FAILED) where maps are cleared and SUSPENDED where members are preserved but sync state is invalidated per RTP5.
ably/realtime/presencemap.py (6)
15-59: LGTM: Correct implementation of RTP2b newness comparison.Properly handles both synthesized messages (timestamp comparison) and normal messages (msgSerial/index comparison) with appropriate edge case handling per the spec.
72-92: LGTM: Well-designed initialization with dependency injection.Good use of configurable functions (member_key_fn, is_newer_fn) and logger for flexibility and testability.
111-157: LGTM: Correct RTP2d implementation with proper newness checking.Properly normalizes actions to PRESENT, handles residual tracking during SYNC, and applies newness checks before updating the map.
159-204: LGTM: Correct RTP2h implementation with SYNC-aware behavior.Properly distinguishes between marking members ABSENT during SYNC (RTP2h2) and removing them outside of SYNC (RTP2h1), with appropriate newness checks.
249-315: LGTM: Comprehensive SYNC lifecycle management.Properly implements RTP18/RTP19 requirements with correct handling of residual and ABSENT members, and appropriate callback invocation for sync completion.
331-351: LGTM: Proper clear() with callback handling to prevent hangs.Critical that sync_complete_callbacks are invoked before clearing to ensure any waiting Futures (from get() with wait_for_sync=True) are resolved and don't hang indefinitely.
AIT-102
Also resolves #649
Adds realtime presence publish and subscribe
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.