Hardening for ledger errors #11950
Hardening for ledger errors #11950
Conversation
| let fakeClock | ||
|
|
||
| before(function () { | ||
| fakeClock = sinon.useFakeTimers() |
There was a problem hiding this comment.
This is needed because code paths hitting setTimeout() will cause unit tests to crash (seemingly randomly) and in some cases hang. When the fake timer is destroyed, all methods registered with setTimeout are cleaned up.
app/browser/api/ledger.js
Outdated
| console.log('ledger client is currently busy; transition will be retried on next launch') | ||
| return | ||
| } | ||
| const delayTime = random.randomInt({ min: 1, max: 500 }) |
There was a problem hiding this comment.
I would add higher values, because now we are setting timeout between 1ms and 500ms
Codecov Report
@@ Coverage Diff @@
## master #11950 +/- ##
==========================================
+ Coverage 53.21% 53.37% +0.15%
==========================================
Files 274 274
Lines 26014 26031 +17
Branches 4167 4169 +2
==========================================
+ Hits 13844 13893 +49
+ Misses 12170 12138 -32
|
bsclifton
force-pushed
the
issue-11945
branch
from
e687b3f to
f9cc082
Compare
…re giving up) Along with previous commits, this fixes #11936 Auditors: @NejcZdovc, @evq includes updated min/max values per #11950 (review)
| @@ -2160,7 +2167,13 @@ const transitionWalletToBat = () => { | |||
| } | |||
|
|
|||
| if (client.busyP()) { | |||
| console.log('ledger client is currently busy; transition will be retried on next launch') | |||
| if (++busyRetryCount > 3) { | |||
| console.log('ledger client is currently busy; transition will be retried on next launch') | |||
There was a problem hiding this comment.
should we hide transition overlay when busy fails for the third time?
There was a problem hiding this comment.
I don't know if it makes sense to hide it without providing a work-around. We don't want to encourage using the old servers indefinitely- I think it would be better to update the transition screen to provide a community link about stuck transitions.
cc: @alexwykoff
|
Is 403 the only case or is it also common for firewalls to redirect to a 'this site is blocked...' with a 200? |
|
@alexwykoff good question- I'm not sure |
|
UPDATE: I confirmed Websense and Barracuda content filters should be using HTTP status code 403 by default for blocked content... however, the DENY action can be customized. The only "true" way to do this would be to fetch a static page on the PIA host and hash it. If that hash is correct, the user has access. Otherwise, they may be on a proxy which is redirecting the content |
...then retry operation with params.useProxy set to false
9c72fba Fixes #11945 Auditors: @mrose17, @NejcZdovc
bsclifton
force-pushed
the
issue-11945
branch
from
f9cc082 to
0cc3eb9
Compare
…re giving up) Along with previous commits, this fixes #11936 Auditors: @NejcZdovc, @evq includes updated min/max values per #11950 (review)
…re giving up) Along with previous commits, this fixes #11936 Auditors: @NejcZdovc, @evq includes updated min/max values per #11950 (review)
bsclifton
force-pushed
the
issue-11945
branch
from
0cc3eb9 to
0778564
Compare
Fixes #11936
Fixes #11945
Changes contain two fixes:
transitionWalletToBat: if busyP returns a truthy value,transitionWalletToBatwill be retried up to 3 times before quitting.Submitter Checklist:
git rebase -ito squash commits (if needed).Reviewer Checklist:
Tests