Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTP17b #835

Merged
merged 20 commits into from
May 22, 2019
Merged

RTP17b #835

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
fff0b4d
RTP17b
ricardopereira Mar 1, 2019
da88c56
Test Suite: HTTPURLResponse allHeaderFields is now case-sensitive
ricardopereira Mar 7, 2019
ed64c01
Log with error convenience method
ricardopereira Mar 7, 2019
ec3a106
RTP17b implementation
ricardopereira Mar 7, 2019
8fb2e0c
Update tests because ARTPresenceMap.isNewestPresence:comparingWith
ricardopereira Mar 7, 2019
3f1bf2d
[fix] Lib is unnecessarily sending another ATTACH
ricardopereira Mar 7, 2019
f3933f7
[fix] Check if response body has data
ricardopereira Mar 8, 2019
8dbc03d
Test Suite: fix [ARTPresenceMessage decodeWithEncoder:error:]: unreco…
ricardopereira Mar 13, 2019
b50cd73
Channel debug log small improvement
ricardopereira Apr 26, 2019
dc84181
syncMsgSerial should only be used when requesting a continue sync
ricardopereira Apr 26, 2019
ca32ef2
Remove duplicated remove local member instruction
ricardopereira Apr 26, 2019
1ea0e4c
Presense Message debug log small fix
ricardopereira Apr 29, 2019
0c2d2f5
fixup! RTP17b
ricardopereira Apr 29, 2019
ec5a7f0
[fix] Should change the sync state after calling sync method
ricardopereira Apr 29, 2019
68b6967
Remove redundant code
ricardopereira Apr 29, 2019
46dd5e7
fixup! RTP17b
ricardopereira May 2, 2019
6058e4a
Merge branch 'develop' into rtp17b
ricardopereira May 6, 2019
782ab1e
[fix] Update Logging test which was improved
ricardopereira May 16, 2019
0d7cdc0
Sync channelSerial should be unset when the SYNC succeeds.
ricardopereira May 21, 2019
9d2ef9e
Test suite: avoid some race conditions
ricardopereira May 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions Source/ARTRealtime.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,12 +550,6 @@ - (ARTEventListener *)transitionSideEffects:(ARTConnectionStateChange *)stateCha

if ([self shouldSendEvents]) {
[self sendQueuedMessages];
// For every Channel
for (ARTRealtimeChannel* channel in self.channels.nosyncIterable) {
if (channel.state_nosync == ARTRealtimeChannelSuspended) {
[channel _attach:nil];
}
Copy link
Contributor Author

@ricardopereira ricardopereira Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
} else if (![self shouldQueueEvents]) {
ARTStatus *channelStatus = status;
if (!channelStatus) {
Expand Down
2 changes: 1 addition & 1 deletion Spec/RealtimeClientChannel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3344,7 +3344,7 @@ class RealtimeClientChannel: QuickSpec {

// Check retry
let start = NSDate()
channel.once(.attaching) { stateChange in
channel.once(.attached) { stateChange in
let end = NSDate()
expect(start).to(beCloseTo(end, within: 1.5))
expect(stateChange?.reason).to(beNil())
Expand Down
5 changes: 1 addition & 4 deletions Spec/RealtimeClientPresence.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,15 +1104,12 @@ class RealtimeClientPresence: QuickSpec {
expect(channel.presenceMap.localMembers).to(haveCount(1))
partialDone()
}
channel.once(.attaching) { stateChange in
channel.once(.attached) { stateChange in
expect(stateChange?.reason).to(beNil())
channel.presence.leave(nil) { error in
expect(error).to(beNil())
partialDone()
}
}
channel.once(.attached) { stateChange in
expect(stateChange?.reason).to(beNil())
partialDone()
}
channel.setSuspended(ARTStatus.state(.ok))
Expand Down