-
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 8 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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,6 +164,59 @@ - (void)internalPostMessages:(id)data callback:(void (^)(ARTErrorInfo *__art_nul | |
} ART_TRY_OR_MOVE_TO_FAILED_END | ||
} | ||
|
||
- (void)sync { | ||
[self sync:nil]; | ||
} | ||
|
||
- (void)sync:(void (^)(ARTErrorInfo *__art_nullable error))callback { | ||
if (callback) { | ||
void (^userCallback)(ARTErrorInfo *__art_nullable error) = callback; | ||
callback = ^(ARTErrorInfo *__art_nullable error) { | ||
ART_EXITING_ABLY_CODE(self->_realtime.rest); | ||
dispatch_async(self->_userQueue, ^{ | ||
userCallback(error); | ||
}); | ||
}; | ||
} | ||
|
||
ART_TRY_OR_MOVE_TO_FAILED_START(_realtime) { | ||
switch (self.state) { | ||
case ARTRealtimeChannelInitialized: | ||
case ARTRealtimeChannelDetaching: | ||
case ARTRealtimeChannelDetached: { | ||
ARTErrorInfo *error = [ARTErrorInfo createWithCode:40000 message:@"unable to sync to channel; not attached"]; | ||
[self.logger logWithError:error]; | ||
if (callback) callback(error); | ||
return; | ||
} | ||
default: | ||
break; | ||
} | ||
|
||
[self.logger verbose:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) requesting a sync operation", _realtime, self, self.name]; | ||
|
||
ARTProtocolMessage * msg = [[ARTProtocolMessage alloc] init]; | ||
msg.action = ARTProtocolMessageSync; | ||
msg.channel = self.name; | ||
|
||
if (self.presenceMap.syncMsgSerial) { | ||
msg.channelSerial = self.presenceMap.syncChannelSerial; | ||
msg.msgSerial = [NSNumber numberWithLongLong:self.presenceMap.syncMsgSerial]; | ||
} | ||
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 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? 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 Thanks for pointing this out. Fixed dc84181. 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. And the second issue (second paragraph of my comment)? 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 Right, fixed 0d7cdc0. Set |
||
|
||
[self.realtime send:msg sentCallback:^(ARTErrorInfo *error) { | ||
if (error) { | ||
[self.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) SYNC request failed with %@", self->_realtime, self, self.name, error]; | ||
callback(error); | ||
} | ||
else { | ||
[self.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) SYNC requested with success", self->_realtime, self, self.name]; | ||
callback(nil); | ||
} | ||
} ackCallback:nil]; | ||
} ART_TRY_OR_MOVE_TO_FAILED_END | ||
} | ||
|
||
- (void)requestContinueSync { | ||
ART_TRY_OR_MOVE_TO_FAILED_START(_realtime) { | ||
[self.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) requesting to continue sync operation after reconnect using msgSerial %lld and channelSerial %@", _realtime, self, self.name, self.presenceMap.syncMsgSerial, self.presenceMap.syncChannelSerial]; | ||
|
@@ -174,9 +227,9 @@ - (void)requestContinueSync { | |
msg.channelSerial = self.presenceMap.syncChannelSerial; | ||
msg.channel = self.name; | ||
|
||
[self.realtime send:msg sentCallback:nil ackCallback:^(ARTStatus *status) { | ||
[self.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) continue sync, status is %@", self->_realtime, self, self.name, status]; | ||
}]; | ||
[self.realtime send:msg sentCallback:^(ARTErrorInfo *error) { | ||
[self.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) continue sync, error is %@", self->_realtime, self, self.name, error]; | ||
} ackCallback:nil]; | ||
} ART_TRY_OR_MOVE_TO_FAILED_END | ||
} | ||
|
||
|
@@ -770,11 +823,11 @@ - (void)onPresence:(ARTProtocolMessage *)message { | |
[self.logger error:@"R:%p C:%p (%@) %@", _realtime, self, self.name, errorInfo.message]; | ||
} | ||
} | ||
|
||
if (!presence.timestamp) { | ||
presence.timestamp = message.timestamp; | ||
} | ||
|
||
if (!presence.id) { | ||
presence.id = [NSString stringWithFormat:@"%@:%d", message.id, i]; | ||
} | ||
|
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