-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fix RTN7c: notify publish callbacks of broken connection. #228
Changes from all commits
163551e
7debb07
ca4a905
714c632
fb5ccdd
2ed75ea
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 |
---|---|---|
|
@@ -140,14 +140,36 @@ - (void)publishProtocolMessage:(ARTProtocolMessage *)pm cb:(ARTStatusCallback)cb | |
} | ||
case ARTRealtimeChannelAttached: | ||
{ | ||
[self.realtime send:pm cb:cb]; | ||
[self sendMessage:pm cb:cb]; | ||
break; | ||
} | ||
default: | ||
NSAssert(NO, @"Invalid State"); | ||
} | ||
} | ||
|
||
|
||
- (void)sendMessage:(ARTProtocolMessage *)pm cb:(ARTStatusCallback)cb { | ||
__block BOOL gotFailure = false; | ||
NSString *oldConnectionId = self.realtime.connection.id; | ||
__block ARTEventListener *listener = [self.realtime.connection on:^(ARTConnectionStateChange *stateChange) { | ||
if (!(stateChange.current == ARTRealtimeClosed || stateChange.current == ARTRealtimeFailed | ||
|| (stateChange.current == ARTRealtimeConnected && ![oldConnectionId isEqual:self.realtime.connection.id] /* connection state lost */))) { | ||
return; | ||
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 think you need to remove the listener as well, right? 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. Maybe not. Line 168 stops the listener. 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've made it a 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 had to revert this at 2ed75ea. The listener only has to be unsubscribed if there's a failure or the message is sent. The connection can change several times before the message callback arrives, e. g. when reconnecting. RealtimeClientConnection and RealtimeClientChannel tests are passing now on my machine, let's see if the whole suite does... (I'm not running all of them in my machine because that basically blocks me from working for half an hour.) |
||
} | ||
gotFailure = true; | ||
[self.realtime.connection off:listener]; | ||
if (!cb) return; | ||
ARTErrorInfo *reason = stateChange.reason ? stateChange.reason : [ARTErrorInfo createWithCode:0 message:@"connection broken before receiving publishing acknowledgement."]; | ||
cb([ARTStatus state:ARTStateError info:reason]); | ||
}]; | ||
|
||
[self.realtime send:pm cb:^(ARTStatus *status) { | ||
[self.realtime.connection off:listener]; | ||
if (cb && !gotFailure) cb(status); | ||
}]; | ||
} | ||
|
||
- (ARTPresenceMap *) presenceMap { | ||
return _presenceMap; | ||
} | ||
|
@@ -446,7 +468,7 @@ - (void)sendQueuedMessages { | |
NSArray *qms = self.queuedMessages; | ||
self.queuedMessages = [NSMutableArray array]; | ||
for (ARTQueuedMessage *qm in qms) { | ||
[self.realtime send:qm.msg cb:qm.cb]; | ||
[self sendMessage:qm.msg cb:qm.cb]; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,8 @@ ART_ASSUME_NONNULL_BEGIN | |
|
||
@property (readonly, getter=getIsConnected) BOOL isConnected; | ||
|
||
@property (readwrite, assign, nonatomic) BOOL closing; | ||
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. Should it be in 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. ARTWebSocketTransport isn't part of the public API anyway. 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. Oh, ok. Sorry 👍 |
||
|
||
- (NSURL *)setupWebSocket:(__GENERIC(NSArray, NSURLQueryItem *) *)params withOptions:(ARTClientOptions *)options resumeKey:(NSString *__art_nullable)resumeKey connectionSerial:(NSNumber *__art_nullable)connectionSerial; | ||
|
||
- (BOOL)getIsConnected; | ||
|
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.
And if the current connection state is FAILED?
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.
What then? The spec says "If a connection enters the CLOSED or FAILED state [...] the client should consider the delivery of those messages as failed", and that's what's happening here. (Maybe this is being done at the wrong level, though, because RTN7 is about ACK and NACK...)