Add optional crash reporter #228
Conversation
gabrielzanoni
force-pushed
the
crash-reporter
branch
from
aec55a1
to
11c4dec
Compare
| completionHandler(disposition, credential); | ||
| } | ||
|
|
||
| #pragma mark NSURLSessionTaskDelegate | ||
|
|
||
| - (void)URLSession:(NSURLSession *)session didBecomeInvalidWithError:(nullable NSError *)error | ||
| { | ||
| [_invalidatedSessions addObject:session]; |
There was a problem hiding this comment.
As an option, we can keep track of invalidated sessions without extra set: a Service owns a SessionSelector, which in turn manages only two (or rather, up to two) session instances, so I guess we could leverage that instead.
Although I'm wondering if a Service should even start performing a request if its only Session has been invalidated. Wdyt?
There was a problem hiding this comment.
Not sure if I'm following why that would be better. This DataLoaderService is the one implementing NSURLSessionTaskDelegate, so if we were to go on that path this service would still have to tell SessionSelector which session is invalidated and then retrieve it after. It seems an unnecessary indirection 🤔 .
About not making a request on an invalidated session, Gianni made a change recently that prevents that from happening on a retry - but we'd still like to know if this happens in a regular request (i.e. leave breadcrumb and crash)
| @property (nonatomic, strong) SPTDataLoaderServerTrustPolicy *serverTrustPolicy; | ||
| @property (nonatomic, weak, nullable) NSFileManager *fileManager; | ||
| @property (nonatomic, weak, nullable) Class dataClass; | ||
| @property (nonatomic, strong) NSMutableSet<NSURLSession *> *invalidatedSessions; |
There was a problem hiding this comment.
I would avoid to add this complexity in keeping track of what session got invalidated.
The invalidateAndCancel method always invalidates all the sessions handled by the SessionSelector.
My suggestion is to keep only a boolean to keep track of the session invalidation. This will simplify this PR
| } else { | ||
| [_crashReporter leaveBreadcrumb: | ||
| [NSString stringWithFormat:@"Trying to retry on invalidated sesion. Policy: %ld, Error: %@", | ||
| (long)handler.request.backgroundPolicy, error.description]]; | ||
| } |
There was a problem hiding this comment.
I am not sure about this one. The retry is an internal feature of SPTDataLoader. The if at line 402/497 was added to fix a bug where SPTDataLoader was trying to retry on an already invalidated sessions. And to add more on it, entering the else part does not guarantee the session was invalidated, it can just be that single task got cancelled and thus the message could be mis-leading as well
There was a problem hiding this comment.
Since this code can't retry on invalidated session anymore, I'll just remove this log then
gabrielzanoni
force-pushed
the
crash-reporter
branch
from
11c4dec
to
2c96b3c
Compare
gabrielzanoni
force-pushed
the
crash-reporter
branch
from
2c96b3c
to
e3ea407
Compare
gabrielzanoni
requested review from
gitrema and
originalWho
and removed request for
gitrema and
originalWho
What has changed
crashReporterof typeSPTDataLoaderCrashReporterto constructorWhy this was changed
This is an attempt to improve crash logging. It takes into account that the app probably has multiple instances of and
SPTDataLoaderduring its lifecycle, which is why the breadcrumbs are left only in critical spots: