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

feat(wallet): query incremental timeout #1395

Closed
wants to merge 3 commits into from

Conversation

kehiy
Copy link
Contributor

@kehiy kehiy commented Jul 2, 2024

Description

This PR creates a time-out for querying data from gRPC on the wallet. we set a timeout for 5 seconds where the default is 30 seconds. we can make sure a slow RPC won't slow down the proccess.

@kehiy kehiy requested a review from b00f July 2, 2024 09:01
@kehiy kehiy force-pushed the wallet-timeout branch from 2131b47 to 12a0853 Compare July 2, 2024 09:09
@kehiy kehiy requested a review from Ja7ad July 2, 2024 09:10
Copy link
Contributor

@Ja7ad Ja7ad left a comment

Choose a reason for hiding this comment

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

for create context not required to call context.WithoutCancel.

ctx := context.WithoutCancel(context.Background())

wallet/client.go Outdated
data, err := trx.Bytes()
if err != nil {
return hash.UndefHash, err
}
res, err := c.transactionClient.BroadcastTransaction(c.ctx,
res, err := c.transactionClient.BroadcastTransaction(ctx,
Copy link
Contributor

@Ja7ad Ja7ad Jul 2, 2024

Choose a reason for hiding this comment

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

Important things is gRPC method BroadcastTransaction should pass context to lower layers to stop process.

When you call gRPC method make a go routine in background to processing then your context send signal cancel base on timeout, process don't canceled in background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should we do to cancel it on background? also, why it won't cancel in background when the timeout reaches?

@kehiy
Copy link
Contributor Author

kehiy commented Jul 2, 2024

for create context not required to call context.WithoutCancel.

ctx := context.WithoutCancel(context.Background())

I'm calling WithTimeout, not WithCancel.

@Ja7ad
Copy link
Contributor

Ja7ad commented Jul 2, 2024

for create context not required to call context.WithoutCancel.

ctx := context.WithoutCancel(context.Background())

I'm calling WithTimeout, not WithCancel.

line 29

@kehiy
Copy link
Contributor Author

kehiy commented Jul 2, 2024

for create context not required to call context.WithoutCancel.

ctx := context.WithoutCancel(context.Background())

I'm calling WithTimeout, not WithCancel.

line 29

this line is not included in this PR, let me update it.

@kehiy kehiy force-pushed the wallet-timeout branch from 2023383 to 12a0853 Compare July 2, 2024 09:38
@kehiy
Copy link
Contributor Author

kehiy commented Jul 2, 2024

@Ja7ad fixed.

@kehiy kehiy requested a review from Ja7ad July 2, 2024 09:39
wallet/client.go Outdated
@@ -76,7 +78,10 @@ func (c *grpcClient) getBlockchainInfo() (*pactus.GetBlockchainInfoResponse, err
return nil, err
}

info, err := c.blockchainClient.GetBlockchainInfo(c.ctx,
ctx, cancel := context.WithTimeout(c.ctx, time.Second*5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please no Kay, Please don't repeat yourself!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems this way of handling the issue it not correct based on @Ja7ad review. once we find the correct way I'll cleanly handle the code. this PR needs change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@b00f @Ja7ad
can you recheck this?
Is there any close method to call cancel there? or should I run a routine to call the cancel based on a condition?

@kehiy kehiy force-pushed the wallet-timeout branch from 62ade4f to c0dc374 Compare July 2, 2024 13:32
@kehiy kehiy requested a review from b00f July 2, 2024 13:32
@Ja7ad
Copy link
Contributor

Ja7ad commented Jul 3, 2024

@kehiy This method is not correct to solve the problem and I will implement another method to solve the problem.

@Ja7ad Ja7ad closed this Jul 3, 2024
@kehiy kehiy deleted the wallet-timeout branch July 3, 2024 07:18
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.

3 participants