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

close wallet connections methods #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

abergasov
Copy link

@abergasov abergasov commented Feb 8, 2024

For cases when connection to dirk go through short-living certifications, current implementation not allow re-load changed certificates because of in-memory pool of grpc connections. Even if certificate was re-loaded and wallet re-created, all connections trigger error because of cached connections

to avoid this added modification - move connection map from global to package to build in to struct PuddleConnectionProvider

to prevent of leak memory for unused old connections was added function CloseConnections

@abergasov abergasov force-pushed the close-wallet-connections branch from 08ff3a0 to fe29ea2 Compare February 8, 2024 19:30
@mcdee
Copy link
Collaborator

mcdee commented Feb 11, 2024

I'm not convinced about have a per-wallet connection pool. We have people using this library with hundreds of wallets, and this would likely lead to resource exhaustion on either server or client. Could this be looked at whilst retaining the global connection pool?

I'd also change CloseConnections to just Close, as a more general "we're finished with this" call?

@abergasov
Copy link
Author

abergasov commented Feb 11, 2024

Sure, I made a slight modification. Now, we can pass an optional parameter via the WithConnectionProviderName method, which will create a dedicated pool of connections for a specific wallet. This change keep backward compatibility for users with hundreds of wallets and does not impact their performance.

Otherwise, it will enable dedicated connections pool for wallets with specific certificate. So, the usage will be like:

Open(
    context.Background(),
    WithName("Test wallet"),
    WithCredentials(credentials.NewTLS(nil)),
    WithEndpoints([]*Endpoint{{host: "localhost", port: 12345}}),
    WithConnectionProviderName(uuid.NewString()), 
)

I'd also change CloseConnections to just Close, as a more general "we're finished with this" call?

Done, now wallet have just Close() method

use global connection pool with optional wallet connection name
@abergasov abergasov force-pushed the close-wallet-connections branch from 5902442 to ba404f1 Compare February 11, 2024 22:07
abergasov added a commit to p2p-org/go-eth2-wallet-dirk that referenced this pull request Feb 14, 2024
abergasov added a commit to p2p-org/go-eth2-wallet-dirk that referenced this pull request Feb 14, 2024
abergasov added a commit to p2p-org/go-eth2-wallet-dirk that referenced this pull request Feb 14, 2024
abergasov added a commit to p2p-org/go-eth2-wallet-dirk that referenced this pull request Feb 14, 2024
abergasov added a commit to p2p-org/go-eth2-wallet-dirk that referenced this pull request May 20, 2024
Copy link
Collaborator

@mcdee mcdee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I lost track of this PR. Please could you rebase it and address the last outstanding item here, and I'll merge it? Thanks.

@@ -67,7 +67,7 @@ func Open(ctx context.Context,
wallet.timeout = parameters.timeout
wallet.endpoints = make([]*Endpoint, len(parameters.endpoints))
wallet.connectionProvider = &PuddleConnectionProvider{
name: parameters.name,
name: parameters.connectionName,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name is mandatory but connectionname is not. So I think that this should use connectionName if present, otherwise it should fall back to using name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants