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.

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

fix(indy): async ledger connection issues on iOS #803

Merged

Conversation

TheTreek
Copy link
Contributor

@TheTreek TheTreek commented May 26, 2022

Change connectToPools function to sequentially connect to pools. Previously there were issues on iOS because of too many open sockets on this function.

Signed-off-by: Patrick Kenyon <treek.kenyon@gmail.com>
@TheTreek TheTreek requested a review from a team as a code owner May 26, 2022 19:25
@TimoGlastra
Copy link
Contributor

How is this PR related to #804? Should we add a note to this code explaining why we're doing it in sequence instead of in parallel? Or is this change unrelated to that change?

@TimoGlastra TimoGlastra changed the title fix: Async ledger connection issues on iOS fix(indy): async ledger connection issues on iOS May 27, 2022
@TimoGlastra
Copy link
Contributor

Will this also fix the issue with connectToIndyLedgersOnStartup?

@codecov-commenter
Copy link

codecov-commenter commented May 30, 2022

Codecov Report

Merging #803 (2b8ac4b) into main (5b42e6c) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #803      +/-   ##
==========================================
- Coverage   87.69%   87.50%   -0.19%     
==========================================
  Files         437      458      +21     
  Lines       10807    11094     +287     
  Branches     1904     1863      -41     
==========================================
+ Hits         9477     9708     +231     
- Misses       1268     1324      +56     
  Partials       62       62              
Impacted Files Coverage Δ
...ore/src/modules/ledger/services/IndyPoolService.ts 100.00% <100.00%> (ø)
packages/core/src/modules/dids/helpers.ts 90.47% <0.00%> (-4.77%) ⬇️
.../connections/handlers/ConnectionResponseHandler.ts 81.25% <0.00%> (-2.09%) ⬇️
...connections/handlers/DidExchangeResponseHandler.ts 80.95% <0.00%> (-1.27%) ⬇️
packages/core/src/agent/MessageSender.ts 84.61% <0.00%> (-1.18%) ⬇️
...les/credentials/protocol/v1/V1CredentialService.ts 87.19% <0.00%> (-1.13%) ⬇️
.../core/src/modules/credentials/CredentialsModule.ts 90.73% <0.00%> (-0.98%) ⬇️
...ckages/core/src/modules/dids/domain/DidDocument.ts 96.51% <0.00%> (-0.83%) ⬇️
...ckages/core/src/modules/routing/RecipientModule.ts 61.85% <0.00%> (-0.65%) ⬇️
...ntials/formats/indy/IndyCredentialFormatService.ts 88.01% <0.00%> (-0.56%) ⬇️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b42e6c...2b8ac4b. Read the comment docs.

@TheTreek
Copy link
Contributor Author

TheTreek commented Jun 6, 2022

How is this PR related to #804? Should we add a note to this code explaining why we're doing it in sequence instead of in parallel? Or is this change unrelated to that change?

The issue we were having is we tried to connect to all ledgers in parallel. This would cause us to reach the resource limit very quickly, so instead of doing it in parallel we do it in sequence so we don't use too many resources.

Will this also fix the issue with connectToIndyLedgersOnStartup?

Yes, this is the main goal of this PR.

@TimoGlastra
Copy link
Contributor

The issue we were having is we tried to connect to all ledgers in parallel. This would cause us to reach the resource limit very quickly, so instead of doing it in parallel we do it in sequence so we don't use too many resources

Can we add a comment to the relevant code explaining this? I'm worried this will get lost and 'fixed' to parallel opening of pools in the future again.

@TimoGlastra
Copy link
Contributor

Yes, this is the main goal of this PR

That is really great to hear!

Signed-off-by: Patrick Kenyon <treek.kenyon@gmail.com>
@TheTreek
Copy link
Contributor Author

TheTreek commented Jun 8, 2022

The issue we were having is we tried to connect to all ledgers in parallel. This would cause us to reach the resource limit very quickly, so instead of doing it in parallel we do it in sequence so we don't use too many resources

Can we add a comment to the relevant code explaining this? I'm worried this will get lost and 'fixed' to parallel opening of pools in the future again.

Done!

@TimoGlastra
Copy link
Contributor

Thansk @TheTreek, if you can update your branch I can merge!

@TimoGlastra
Copy link
Contributor

Sorry it's out of date again :( Can you update again?

@TimoGlastra TimoGlastra enabled auto-merge (squash) June 9, 2022 21:02
@TimoGlastra TimoGlastra merged commit 8055652 into openwallet-foundation:main Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants