Fix FIAM test flakes and exposed library bugs #3101
Conversation
|
Tests ran cleanly except for #3102 |
| @@ -105,9 +105,6 @@ - (instancetype)initWithRequestSender:(FIRIAMClearcutHttpRequestSender *)request | |||
| _nextValidSendTimeInMills = (int64_t) | |||
| [_userDefaults doubleForKey:FIRIAM_UserDefaultsKeyForNextValidClearcutUploadTimeInMills]; | |||
|
|
|||
| // seed the first send upon SDK start-up | |||
| [self scheduleNextSend]; | |||
There was a problem hiding this comment.
Will want to hear back from @christibbs about this one. We may want to do this in case there are actually pending logs that are written to disk?
There was a problem hiding this comment.
Trying out an approach where we only call this if there are pending logs to be sent.
| @@ -205,15 +202,6 @@ - (void)attemptUploading { | |||
| } | |||
|
|
|||
| - (void)scheduleNextSend { | |||
| @synchronized(self) { | |||
| if (_nextSendScheduled) { | |||
There was a problem hiding this comment.
Don't we want to keep this, checking if _nextSendScheduled is already set? I noticed in the log you mentioned it could cause an issue, but is the issue because of this check AND the scheduleNextSend in the init or just the one?
There was a problem hiding this comment.
I added this check back, but I'm not sure what _nextSendScheduled = YES in the else block was doing.. it's already being set to YES at the end of the method. I removed the else block.
There was a problem hiding this comment.
I think this could still cause problems if you want to change waitTime. The new waitTime will get ignored for the next scheduled send. Perhaps that's ok?
There was a problem hiding this comment.
I think that should be fine. The timing of sending logs doesn't need to be precise in this case.
…File. Move mock setup of mockTimeFetcher prior to initializing FIRIAMClearcutLogStorage
|
LGTM but want to confirm that the |
|
@ryanwilson I'm testing out an approach where it only schedules the next send on |
…are logs that haven't been sent at that time. Early return from scheduling if there's already a pending send
Fix #3046.
self.mockTimeFetcher currentTimestampInSecondsstub before the mock is passed to other functions.scheduleNextSendin the initializer since the first zero-delayed log can happen logs to happen to soon.scheduleNextSendsince it can cause logs to get dropped if the first zero-length log is scheduled, the next real log gets scheduled, but the log has already been dropped because of no packets available.