-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RTP17b #835
RTP17b #835
Changes from all commits
fff0b4d
da88c56
ed64c01
ec3a106
8fb2e0c
3f1bf2d
f3933f7
8dbc03d
b50cd73
dc84181
ca32ef2
1ea0e4c
0c2d2f5
ec5a7f0
68b6967
46dd5e7
6058e4a
782ab1e
0d7cdc0
9d2ef9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@SimonWoolf Yes it does. When I implemented ably-cocoa/Source/ARTRealtime.m Lines 608 to 615 in 8dbc03d
ably-cocoa/Source/ARTRealtimeChannel.m Lines 924 to 934 in 8dbc03d
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@SimonWoolf That's because the test forces to suspend: ably-cocoa/Spec/RealtimeClientPresence.swift Lines 3034 to 3039 in 8dbc03d
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍 Though looking at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SimonWoolf You're not misreading. It seems so, it's not respecting |
||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} else if (![self shouldQueueEvents]) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
ARTStatus *channelStatus = status; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!channelStatus) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you've added this, you need to remove line 134, or it'll do the remove unconditionally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed ca32ef2. Thanks