RTP17b #835

ricardopereira · 2019-03-01T07:53:57Z

(RTP17b) The events that should be applied to the RTP17 presence map are:

any ENTER, PRESENT or UPDATE event with a connectionId that matches the current client’s connectionId;

any LEAVE event with a connectionId that matches the current client’s connectionId and is NOT a ‘synthesized leave’ (an event that has a connectionId which is not an initial substring of its id, per RTP2b1 )

Related #737

Swift Regression https://bugs.swift.org/browse/SR-2429 That's why sometimes some tests were failing because the server responded differently: X-Ably-Errorcode or X-Ably-errorcode, etc.

is now ARTPresenceMessage.isNewerThan.

when channel is Suspended. Realtime sends an ATTACHED message after a successful resume.

ricardopereira · 2019-03-07T16:15:29Z

Source/ARTRealtime.m

-        for (ARTRealtimeChannel* channel in self.channels.nosyncIterable) {
-            if (channel.state_nosync == ARTRealtimeChannelSuspended) {
-                [channel _attach:nil];
-            }


I did this fix in this PR because the RTP17b test was failing because of this. Discussion here.

@ricardopereira With this removal, does the library comply with RTN15c3? "when a resume request results in a CONNECTED with a new connectionId ... The client library should initiate an attach for channels that are in the SUSPENDED state. For all channels in the ATTACHING or ATTACHED state, the client library should fail any previously queued messages for that channel and initiate a new attach..."

cf my comment in slack "you shouldn't be sending an attached after a successful resume, only for an unsuccessful one" -- you still need to do the latter.

(From that test log it looks like the connection moved to suspended, but it's not obvious why from the log, the resume was successful so I can't see any reason for it to do so)

With this removal, does the library comply with RTN15c3?

@SimonWoolf Yes it does. When I implemented RTN15c3, I forgot to remove that bit because the new implementation already handle that case. You can see that the channel will reattach

ably-cocoa/Source/ARTRealtime.m

Lines 608 to 615 in 8dbc03d

if (![message.connectionId isEqualToString:self.connection.id_nosync]) {

[self.logger warn:@"R:%p ARTRealtime: connection has reconnected, but resume failed. Reattaching any attached channels", self];

// Reattach all channels

for (ARTRealtimeChannel *channel in self.channels.nosyncIterable) {

[channel reattachWithReason:message.error callback:nil];

}

_resuming = false;

}

and it does check the SUSPENDED channel state

ably-cocoa/Source/ARTRealtimeChannel.m

Lines 924 to 934 in 8dbc03d

case ARTRealtimeChannelSuspended:

[self.realtime.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) attached or suspended and will reattach", _realtime, self, self.name];

break;

case ARTRealtimeChannelAttaching:

[self.realtime.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) already attaching", _realtime, self, self.name];

if (callback) [_attachedEventEmitter once:callback];

return;

default:

break;

}

[self internalAttach:callback withReason:reason storeErrorInfo:false];

(From that test log it looks like the connection moved to suspended, but it's not obvious why from the log, the resume was successful so I can't see any reason for it to do so)

@SimonWoolf That's because the test forces to suspend:

ably-cocoa/Spec/RealtimeClientPresence.swift

Lines 3034 to 3039 in 8dbc03d

// Inject an additional member into the myMember set, then force a suspended state

client.simulateSuspended(beforeSuspension: { done in

channel.presenceMap.localMembers.add(additionalMember)

done()

})

expect(client.connection.state).to(equal(.suspended))

I forgot to remove that bit because the new implementation already handle that case

👍

Though looking at reattachWithReason, it looks to me like it's reattaching channels in every state except attaching. So (1) it's starting an attach for channels in initialized, detached, or failed, which it really shouldn't be; and (2) it isn't initiating a new attach for a channel in the attaching state (we've just started a completely new connection, so any previously-in-progress attach needs to be started again). Or am I misreading it?

@SimonWoolf You're not misreading. It seems so, it's not respecting RTN15c3. Issue has been created: #847

Case: Push test was failing because lib was returning an error with 204 status code which shouldn't.

…gnized selector sent to instance Because of 8fb2e0c, there are 2 ARTBaseMessage (ARTPresenceMessage) class method swizzling and Aspects doesn't support that and that's why the error occurs. There are similar issues in the Aspects repo like this one: Aspects/issues/145. I tried the fix of that PR but it didn't work. So, I decided to change all of the class methods swizzling to fix this case and reduce future issues.

SimonWoolf

The actual RTP17b bit (newer comparison etc.) looks good, but some questions re the sync() method and the channel reattaching bit

SimonWoolf · 2019-04-24T10:21:08Z

Source/ARTPresenceMap.m

@@ -119,6 +120,10 @@ - (void)internalRemove:(ARTPresenceMessage *)message {
 }

 - (void)internalRemove:(ARTPresenceMessage *)message force:(BOOL)force {
+    if ([message.connectionId isEqualToString:self.delegate.connectionId] && !message.isSynthesized) {
+        [_localMembers removeObject:message];


Now you've added this, you need to remove line 134, or it'll do the remove unconditionally

Fixed ca32ef2. Thanks

SimonWoolf · 2019-04-24T10:34:56Z

Source/ARTRealtime.m

-        for (ARTRealtimeChannel* channel in self.channels.nosyncIterable) {
-            if (channel.state_nosync == ARTRealtimeChannelSuspended) {
-                [channel _attach:nil];
-            }


@ricardopereira With this removal, does the library comply with RTN15c3? "when a resume request results in a CONNECTED with a new connectionId ... The client library should initiate an attach for channels that are in the SUSPENDED state. For all channels in the ATTACHING or ATTACHED state, the client library should fail any previously queued messages for that channel and initiate a new attach..."

cf my comment in slack "you shouldn't be sending an attached after a successful resume, only for an unsuccessful one" -- you still need to do the latter.

(From that test log it looks like the connection moved to suspended, but it's not obvious why from the log, the resume was successful so I can't see any reason for it to do so)

SimonWoolf · 2019-04-24T10:51:15Z

Source/ARTRealtimeChannel.m

+    if (self.presenceMap.syncMsgSerial) {
+        msg.channelSerial = self.presenceMap.syncChannelSerial;
+        msg.msgSerial = [NSNumber numberWithLongLong:self.presenceMap.syncMsgSerial];
+    }


I don't understand this. What is a syncMsgSerial? It looks like it's being set from a SYNC msgSerial, but that isn't a thing (no incoming messages ever have msgSerials, and SYNCs don't have them in either direction). Nothing in RTP3 tells you to do this, it only asks for the channelSerial.

Also, AFAICS syncChannelSerial and syncMsgSerial are never unset in endSync, so any sync() call will result in the lib trying to resume an actually-successfully-completed sync, no?

@SimonWoolf Thanks for pointing this out. Fixed dc84181.

And the second issue (second paragraph of my comment)? presenceMap.syncChannelSerial is only ever set, never unset, and you're setting it on the sync unconditionally, requesting a resume of a sync that actually completed successfully?

@SimonWoolf Right, fixed 0d7cdc0. Set syncChannelSerial to nil when the sync ends.

SimonWoolf · 2019-04-24T10:55:45Z

Spec/RealtimeClientPresence.swift

+                        }
+                    }
+                }
+


I can't see the bit of this test that tests synthesized leave's effect on the own-members set, as the test name implies? (If it is there than it's odd that it passed, since https://github.com/ably/ably-cocoa/pull/835/files#r278056029 means that the logic wasn't actually taking effect)

@SimonWoolf You're right, that part is missing. What's the best way to test the synthesized leave's effect? I'm thinking in forcing a synthesized leave of one existing presence and force a sync. Then after the sync, the user should be updated since the user is re-entered and the final members map should remain the same, no leave has taken effect.

A proper end-to-end test that forces realtime to do a synthesized leave is a bit annoying if only because it means the test will take at least 15 seconds. But if you want to do that, something like: enter, force a disconnect, wait 20s, reconnect, wait to see the synthesized leave (& sync showing the member not present), wait for lib to automatically re-enter.

@SimonWoolf Thanks, updated the test case 46dd5e7.

ricardopereira · 2019-05-03T07:37:16Z

~~It needs #845 for the test suite pass.~~

Branch updated.

ricardopereira added 6 commits March 1, 2019 07:32

RTP17b

fff0b4d

Test Suite: HTTPURLResponse allHeaderFields is now case-sensitive

da88c56

Swift Regression https://bugs.swift.org/browse/SR-2429 That's why sometimes some tests were failing because the server responded differently: X-Ably-Errorcode or X-Ably-errorcode, etc.

Log with error convenience method

ed64c01

RTP17b implementation

ec3a106

Update tests because ARTPresenceMap.isNewestPresence:comparingWith

8fb2e0c

is now ARTPresenceMessage.isNewerThan.

[fix] Lib is unnecessarily sending another ATTACH

3f1bf2d

when channel is Suspended. Realtime sends an ATTACHED message after a successful resume.

ricardopereira commented Mar 7, 2019

View reviewed changes

ricardopereira marked this pull request as ready for review March 7, 2019 17:21

ricardopereira added 2 commits March 8, 2019 17:49

[fix] Check if response body has data

f3933f7

Case: Push test was failing because lib was returning an error with 204 status code which shouldn't.

ricardopereira requested a review from SimonWoolf March 15, 2019 07:06

SimonWoolf requested changes Apr 24, 2019

View reviewed changes

SimonWoolf reviewed Apr 24, 2019

View reviewed changes

ricardopereira added 8 commits April 26, 2019 11:47

Channel debug log small improvement

b50cd73

syncMsgSerial should only be used when requesting a continue sync

dc84181

Remove duplicated remove local member instruction

ca32ef2

Presense Message debug log small fix

1ea0e4c

fixup! RTP17b

0c2d2f5

[fix] Should change the sync state after calling sync method

ec5a7f0

Remove redundant code

68b6967

fixup! RTP17b

46dd5e7

ricardopereira mentioned this pull request May 2, 2019

Check RTN15c3 implementation #847

Closed

ricardopereira requested a review from SimonWoolf May 3, 2019 07:37

ricardopereira and others added 4 commits May 6, 2019 15:57

Merge branch 'develop' into rtp17b

6058e4a

[fix] Update Logging test which was improved

782ab1e

Sync channelSerial should be unset when the SYNC succeeds.

0d7cdc0

Test suite: avoid some race conditions

9d2ef9e

SimonWoolf approved these changes May 22, 2019

View reviewed changes

ricardopereira merged commit a926e4d into develop May 22, 2019

ricardopereira deleted the rtp17b branch May 22, 2019 09:30

maratal mentioned this pull request Aug 27, 2021

Reattaching channels in compliance with RTN15c3 #1172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTP17b #835

RTP17b #835

ricardopereira commented Mar 1, 2019 •

edited

Loading

ricardopereira Mar 7, 2019 •

edited

Loading

SimonWoolf Apr 24, 2019

ricardopereira Apr 26, 2019

ricardopereira Apr 26, 2019

SimonWoolf Apr 26, 2019

ricardopereira May 2, 2019

SimonWoolf left a comment

SimonWoolf Apr 24, 2019

ricardopereira May 2, 2019

SimonWoolf Apr 24, 2019

SimonWoolf Apr 24, 2019

ricardopereira May 2, 2019

SimonWoolf May 21, 2019

ricardopereira May 21, 2019

SimonWoolf Apr 24, 2019

ricardopereira Apr 26, 2019

SimonWoolf Apr 30, 2019

ricardopereira May 2, 2019

ricardopereira commented May 3, 2019 •

edited

Loading

	if (![message.connectionId isEqualToString:self.connection.id_nosync]) {
	[self.logger warn:@"R:%p ARTRealtime: connection has reconnected, but resume failed. Reattaching any attached channels", self];
	// Reattach all channels
	for (ARTRealtimeChannel *channel in self.channels.nosyncIterable) {
	[channel reattachWithReason:message.error callback:nil];
	}
	_resuming = false;
	}

	case ARTRealtimeChannelSuspended:
	[self.realtime.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) attached or suspended and will reattach", _realtime, self, self.name];
	break;
	case ARTRealtimeChannelAttaching:
	[self.realtime.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) already attaching", _realtime, self, self.name];
	if (callback) [_attachedEventEmitter once:callback];
	return;
	default:
	break;
	}
	[self internalAttach:callback withReason:reason storeErrorInfo:false];

	// Inject an additional member into the myMember set, then force a suspended state
	client.simulateSuspended(beforeSuspension: { done in
	channel.presenceMap.localMembers.add(additionalMember)
	done()
	})
	expect(client.connection.state).to(equal(.suspended))

RTP17b #835

RTP17b #835

Conversation

ricardopereira commented Mar 1, 2019 • edited Loading

ricardopereira Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonWoolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardopereira commented May 3, 2019 • edited Loading

ricardopereira commented Mar 1, 2019 •

edited

Loading

ricardopereira Mar 7, 2019 •

edited

Loading

ricardopereira commented May 3, 2019 •

edited

Loading