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

Add extra logging and always cancel eventStreamRequest before setting to nil #1597

Closed
wants to merge 1 commit into from
Closed

Add extra logging and always cancel eventStreamRequest before setting to nil #1597

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 14 additions & 2 deletions MatrixSDK/MXSession.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,6 +1097,7 @@ - (void)pause
// Cancel the current request managing the event stream
[eventStreamRequest cancel];
eventStreamRequest = nil;
MXLogDebug(@"[MXSession] pause: did cancel and remove eventStreamRequest");

for (MXPeekingRoom *peekingRoom in peekingRooms)
{
Expand Down Expand Up @@ -1203,6 +1204,7 @@ - (BOOL)reconnect
MXLogDebug(@"[MXSession] Reconnect starts");
[eventStreamRequest cancel];
eventStreamRequest = nil;
MXLogDebug(@"[MXSession] reconnect: did cancel and remove eventStreamRequest");

// retrieve the available data asap
// disable the long poll to get the available data asap
Expand All @@ -1223,6 +1225,7 @@ - (void)close
// Cancel the current server request (if any)
[eventStreamRequest cancel];
eventStreamRequest = nil;
MXLogDebug(@"[MXSession] close: did cancel and remove eventStreamRequest");

// Flush pending direct room operations
[directRoomsOperationsQueue removeAllObjects];
Expand Down Expand Up @@ -1480,6 +1483,7 @@ - (void)serverSyncWithServerTimeout:(NSUInteger)serverTimeout
} failure:^(NSError *error) {
[self handleServerSyncError:error forRequestWithServerTimeout:serverTimeout success:success failure:failure];
}];
MXLogDebug(@"[MXSession] serverSyncWithServerTimeout: did create eventStreamRequest");
}

dispatch_group_notify(initialSyncDispatchGroup, dispatch_get_main_queue(), ^{
Expand Down Expand Up @@ -1557,15 +1561,17 @@ - (void)serverSyncWithServerTimeout:(NSUInteger)serverTimeout
if (self.state == MXSessionStateBackgroundSyncInProgress)
{
// Check that none required the session to keep running
if (self.preventPauseCount)
if (self.preventPauseCount > 0)
{
// Delay the pause by calling the reliable `pause` method.
[self pause];
}
else
{
MXLogDebug(@"[MXSession] go to paused ");
[self->eventStreamRequest cancel];
self->eventStreamRequest = nil;
MXLogDebug(@"[MXSession] serverSyncWithServerTimeout onBackgroundSyncDone: did cancel and remove eventStreamRequest");
[self setState:MXSessionStatePaused];
return;
}
Expand Down Expand Up @@ -1650,15 +1656,17 @@ - (void)handleServerSyncError:(NSError*)error forRequestWithServerTimeout:(NSUIn
if (self.state == MXSessionStateBackgroundSyncInProgress)
{
// Check that none required the session to keep running
if (self.preventPauseCount)
if (self.preventPauseCount > 0)
{
// Delay the pause by calling the reliable `pause` method.
[self pause];
}
else
{
MXLogDebug(@"[MXSession] go to paused ");
[self->eventStreamRequest cancel];
self->eventStreamRequest = nil;
MXLogDebug(@"[MXSession] onBackgroundSyncFail: did cancel and remove eventStreamRequest");
[self setState:MXSessionStatePaused];
return;
}
Expand All @@ -1672,6 +1680,9 @@ - (void)handleServerSyncError:(NSError*)error forRequestWithServerTimeout:(NSUIn
// Check whether the caller wants to handle error himself
if (failure)
{
MXLogErrorDetails(@"[MXSession] handleServerSyncError: sync fail is handled by caller, resetting eventStreamRequest might not be handled", @{
@"error": error ?: @"unknown"
});
Comment on lines +1683 to +1685
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ismailgulek WDYT about this bit ? If the error is a timeout and failure != nil, the eventStreamRequest might not be reset 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah you right. It even makes sense to reset the request here regardless of failure block.

failure(error);
}
else
Expand All @@ -1695,6 +1706,7 @@ - (void)handleServerSyncError:(NSError*)error forRequestWithServerTimeout:(NSUIn
// The reconnection attempt failed on timeout: there is no data to retrieve from server
[self->eventStreamRequest cancel];
self->eventStreamRequest = nil;
MXLogDebug(@"[MXSession] handleServerSyncError onBackgroundSyncTimeout: did cancel and remove eventStreamRequest");

// Notify the reconnection attempt has been done.
[[NSNotificationCenter defaultCenter] postNotificationName:kMXSessionDidSyncNotification
Expand Down