fix(login): put slow operations in go routines to speed up login#7349
fix(login): put slow operations in go routines to speed up login#7349jrainville wants to merge 1 commit intodevelopfrom
Conversation
|
Jenkins BuildsClick to see older builds (71)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7349 +/- ##
============================================
- Coverage 61.02% 39.38% -21.64%
============================================
Files 824 812 -12
Lines 116026 113849 -2177
============================================
- Hits 70805 44840 -25965
- Misses 37977 63279 +25302
+ Partials 7244 5730 -1514
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jrainville
force-pushed
the
fix/slow-login-on-bad-connection
branch
2 times, most recently
from
c8e5b36 to
86dd634
Compare
|
@saledjenic can you review please |
jrainville
force-pushed
the
fix/slow-login-on-bad-connection
branch
4 times, most recently
from
f67d3f6 to
0531470
Compare
| } | ||
|
|
||
| s.runPeriodically(ctx, s.updateOffset, err == nil) | ||
| go func() { |
There was a problem hiding this comment.
@jrainville this is not the code that I am familiar with, actually I see it for the first time now, but just by checking what runPeriodically does I see at least one issue here and it is that the first check in that function:
if s.started {
return
}since you added s.runPeriodically to Go routine, it means that line 265 s.started = true is always executed before line 262 and runPeriodically never executes because of the first check there.
So we need to do changes more carefully here, also, I see that this computes a time offset from NTP servers, which is crucial for accepting/rejecting messages, which means this is very important and not sure that the right time will be fetched and used if we delay calls in Go routine. But also would be good to ask @osmaczko and @igor-sirotin for review also, since they were touching this code.
There was a problem hiding this comment.
Very good find. I moved the started flag. I'll check with Patryk to see if it's an issue.
I did a quick test and the offset at first is 0s and then 0.5 seconds later it was updated to -2.429584ms, which is very small. I wonder if it could cause issues with some computers though
| if err == nil { | ||
| _ = s.db.UpdateCachedFormats(fiatFormats) | ||
| } | ||
| go func() { |
There was a problem hiding this comment.
In this Go routine no race conditions as I see, but it would be good to add the same proper exit check as we have in the Go routine below:
select {
case <-ctx.Done():
return
default:
}And another problem could be, but we should check that (maybe only new Status profiles are affected, but maybe not cause the login process takes some time), since UpdateCachedFormats populates the db, which is empty for new profiles, that the client may read empty currency formats before the table gets populated.
There was a problem hiding this comment.
In this Go routine no race conditions as I see, but it would be good to add the same proper exit check as we have in the Go routine below
Since this new one doesn't have a ticker, it doesn't need a Done. It's only needed when it's a repeating routine.
since UpdateCachedFormats populates the db, which is empty for new profiles, that the client may read empty currency formats before the table gets populated.
I'll test it. I'll put a sleep in the routine to fake a slow load and then check if I have currencies.
There was a problem hiding this comment.
I just tested and it seems ok
Needed for status-im/status-app#19856 During my investigation, I found that these two services take a long time to initiate and block the login process. Putting these calls in go routines fixes the issue
jrainville
force-pushed
the
fix/slow-login-on-bad-connection
branch
from
0531470 to
adf8859
Compare
3 participants
Needed for status-im/status-app#19856
During my investigation, I found that these two services take a long time to initiate and block the login process. Putting these calls in go routines fixes the issue
status-app PR: status-im/status-app#20144
See the link above for videos