Problem: walletconnnect requests would not be cleared if no response (fix #302) #303

damoncro · 2022-12-08T02:52:10Z

Close:

Problem: walletconnnect requests would not be cleared if no response #302

Solution:

Add pending_requests_timeout and pending_requests_limit

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

Thank you for your code, it's appreciated! :)

damoncro · 2022-12-08T03:28:15Z

Unreal test logs:

LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignPersonal Error: sign_personal error Reached the limit (2) pending requests, please clear all pending requests before making new requests
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] C1E3555516984B53C275264AE159CCC5804279D9DEA0C6CA9DBA80DDE993C8CA3016665687A30FB6FDB8B99B54DBC48AB39D0DE22A9D721B0C2D059B7115E9FD1C
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] 
LogBlueprintUserMessages: [ConnectWalletConnect_C_0] PlayCppSdk SignEip155Transaction Error: sign_typed_transaction error client error: channel closed

tomtau

I haven't read the original code in a while and feel a bit sick today. the patch looks ok, but can't confirm for 100%

wallet-connect/src/client/socket.rs

codecov-commenter · 2022-12-08T05:02:57Z

Codecov Report

Merging #303 (d3979dc) into main (9998589) will decrease coverage by 0.32%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
- Coverage   25.59%   25.27%   -0.33%     
==========================================
  Files          20       20              
  Lines        2188     2216      +28     
==========================================
  Hits          560      560              
- Misses       1628     1656      +28

Impacted Files	Coverage Δ
wallet-connect/src/client/core.rs	`8.65% <0.00%> (-0.17%)`	⬇️
wallet-connect/src/client/socket.rs	`0.00% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

leejw51crypto · 2022-12-08T07:14:29Z

if blocking is 10 minutes, how can it be stopped?
it's sleeping for the duration, when user successfully finished signing, how sleeping thread stopped?

damoncro · 2022-12-08T08:14:51Z

if blocking is 10 minutes, how can it be stopped?

it's sleeping for the duration, when user successfully finished signing, how sleeping thread stopped?

It is hardcoded as 1 minute. What operations could be 10 minutes?

damoncro · 2022-12-08T08:15:40Z

if blocking is 10 minutes, how can it be stopped?

it's sleeping for the duration, when user successfully finished signing, how sleeping thread stopped?

The clear thread would not stop, if succeeded, pending_requests would be removed on the normal thread, while on the clear thread, calling remove on pending_reqeusts would return None. Nothing would happen.

leejw51crypto · 2022-12-08T10:11:23Z

ok

leejw51crypto · 2022-12-08T10:19:25Z

instead sleep,
how about polling with 1 second interval?
while for maxium waitiing time
{
// check pending task
// if it's done
// exit, finisht thread
}
// do current

leejw51crypto · 2022-12-08T10:20:23Z

because, it waits let's say 1 minute sleep,
spawing thread can be too many

damoncro · 2022-12-09T02:21:46Z

instead sleep, how about polling with 1 second interval? while for maxium waitiing time { // check pending task // if it's done // exit, finisht thread } // do current

It is a different purpose. This PR is about cleaning up the pending request no matter it is valid or not, after timeout, to handle no response case (thread may never end if we only query it and wait it to end).

damoncro · 2022-12-09T02:23:49Z

because, it waits let's say 1 minute sleep, spawing thread can be too many

Spawning would not be too many, it has pending_requests_limit which is 2, only 2 at most is permitted. If 3 or more, game will halt, as I reported on the issue.

leejw51crypto · 2022-12-09T03:16:31Z

socket sender is changed to bounded, but it can cause waiting , if there are multiple requests.
how about using unbounded channel, and check count beforehand,
return false?

leejw51crypto · 2022-12-09T03:21:33Z

and

                    (Ok(_), Some(id)) => {
                        // clean up pending request after timeout
                        let _ = tokio::spawn(async move {

if signing is done fast, how this async finishes?

damoncro · 2022-12-09T03:26:38Z

socket sender is changed to bounded, but it can cause waiting , if there are multiple requests. how about using unbounded channel, and check count beforehand, return false?

Bound is enough, and more performant, bounded buffer size is defined by pending_requests_limit. As only 2 requests
at most are permitted, it is not necessary to use unbounded.

leejw51crypto · 2022-12-09T03:28:18Z

yes, if called , 3 times, 3rd connect will keep waiting until it can send.

and socket.rs 258 line

(Ok(_), Some(id)) => {
                        // clean up pending request after timeout
                        let _ = tokio::spawn(async move {
                            let context = context.clone();                            sleep(Duration::from_millis(context.pending_requests_timeout)).await;

this will be alive , no matter succeed or not, for pending_requests_timeout time

so this can cause dead-lock in application side.

damoncro · 2022-12-09T03:29:04Z

and

                    (Ok(_), Some(id)) => {
                        // clean up pending request after timeout
                        let _ = tokio::spawn(async move {

if signing is done fast, how this async finishes?

It will still run, after timeout, it would return None. Since the corresponding request of that id is already removed, please check https://github.com/cronos-labs/play-cpp-sdk/blob/main/wallet-connect/src/client/socket.rs#L54

leejw51crypto · 2022-12-09T03:32:08Z

yes, it's already removed at the stage.
but it will wait anyway for that time whether finished or not.

damoncro · 2022-12-09T03:39:26Z

yes, it's already removed at the stage. but it will wait anyway for that time whether finished or not.

Let me check if close the timeout thread if got response.

leejw51crypto · 2022-12-09T03:43:36Z

ok, i'll read more.
i'm looking into any side-effect

damoncro · 2022-12-09T03:50:27Z

yes, if called , 3 times, 3rd connect will keep waiting until it can send.

This would not happen. Since https://github.com/cronos-labs/play-cpp-sdk/pull/303/files#diff-0fb0fbb1ca52eaa6fa7aa5c4b0958e9b85d6907737ac1fd25c1e48ce90205691R126 already limit it to at most 2. Please see the example and verify.

Besides, deadlock would not happen since no mutex is used here.

damoncro · 2022-12-09T03:51:34Z

yes, it's already removed at the stage. but it will wait anyway for that time whether finished or not.

I agree with this, the timeout thread should be closed if got response. Let me do this.

fix (cronos-labs#302) Close: - cronos-labs#302 Solution: - Add pending_requests_timeout and pending_requests_limit - Use channel instead of unbounded_channel to improve performance

damoncro · 2022-12-12T04:55:07Z

https://github.com/cronos-labs/play-cpp-sdk/blob/main/wallet-connect/src/client/socket.rs#L182
In create_session, I guess, we may not need to make it timeout? @leejw51crypto @tomtau

Or we could make the timeout long, long enough to let the user finish the first connection? Or make a new QR expired timeout for user to configure?

tomtau · 2022-12-12T05:12:39Z

https://github.com/cronos-labs/play-cpp-sdk/blob/main/wallet-connect/src/client/socket.rs#L182 In create_session, I guess, we may not need to make it timeout? @leejw51crypto @tomtau

Or we could make the timeout long, long enough to let the user finish the first connection? Or make a new QR expired timeout for user to configure?

maybe easiest not to timeout? one can still try to recreate/restart if it fails?

tomtau

quite simple and nice with the timeout... just a nit that duration could just be stored in the struct directly

wallet-connect/src/client/core.rs

wallet-connect/src/client/socket.rs

leejw51crypto · 2022-12-12T06:30:52Z

make 60000 as definition

leejw51crypto

lgtm

leejw51crypto · 2022-12-12T06:33:30Z

i think current approach is simplest

damoncro requested review from tomtau and leejw51crypto December 8, 2022 02:52

damoncro mentioned this pull request Dec 8, 2022

Problem: Blocking operation on AsyncTask would not stop makes program halt cronos-labs/play-unreal-plugin#215

Open

tomtau reviewed Dec 8, 2022

View reviewed changes

wallet-connect/src/client/socket.rs Outdated Show resolved Hide resolved

damoncro changed the title ~~Problem: walletconnnect requests would not be cleared if no response fix (#302)~~ Problem: walletconnnect requests would not be cleared if no response (fix #302) Dec 8, 2022

damoncro marked this pull request as draft December 9, 2022 04:34

damoncro added 3 commits December 9, 2022 18:25

Problem: walletconnnect requests would not be cleared if no response

b970ed3

fix (cronos-labs#302) Close: - cronos-labs#302 Solution: - Add pending_requests_timeout and pending_requests_limit - Use channel instead of unbounded_channel to improve performance

Add detailed timeout verbose

532e608

Back to unbounded_channel

94b8593

damoncro force-pushed the bug/wc_request_do_not_clear branch from fc848e1 to 94b8593 Compare December 9, 2022 10:25

damoncro added 2 commits December 12, 2022 12:36

Use timeout wrapper instead of a new thread

a951e08

Back to sync

3617584

damoncro marked this pull request as ready for review December 12, 2022 04:40

Update the comment doc

6e21bff

tomtau approved these changes Dec 12, 2022

View reviewed changes

wallet-connect/src/client/core.rs Outdated Show resolved Hide resolved

wallet-connect/src/client/socket.rs Show resolved Hide resolved

damoncro added 2 commits December 12, 2022 14:02

Use Duration instead

8335fbc

Use u64 instead

d3979dc

leejw51crypto approved these changes Dec 12, 2022

View reviewed changes

leejw51crypto merged commit 495e0cc into cronos-labs:main Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: walletconnnect requests would not be cleared if no response (fix #302) #303

Problem: walletconnnect requests would not be cleared if no response (fix #302) #303

damoncro commented Dec 8, 2022 •

edited

Loading

damoncro commented Dec 8, 2022 •

edited

Loading

tomtau left a comment

codecov-commenter commented Dec 8, 2022 •

edited

Loading

leejw51crypto commented Dec 8, 2022 •

edited

Loading

damoncro commented Dec 8, 2022

damoncro commented Dec 8, 2022 •

edited

Loading

leejw51crypto commented Dec 8, 2022

leejw51crypto commented Dec 8, 2022

leejw51crypto commented Dec 8, 2022

damoncro commented Dec 9, 2022 •

edited

Loading

damoncro commented Dec 9, 2022 •

edited

Loading

leejw51crypto commented Dec 9, 2022 •

edited

Loading

leejw51crypto commented Dec 9, 2022

damoncro commented Dec 9, 2022

leejw51crypto commented Dec 9, 2022 •

edited

Loading

damoncro commented Dec 9, 2022

leejw51crypto commented Dec 9, 2022

damoncro commented Dec 9, 2022

leejw51crypto commented Dec 9, 2022

damoncro commented Dec 9, 2022 •

edited

Loading

damoncro commented Dec 9, 2022

damoncro commented Dec 12, 2022 •

edited

Loading

tomtau commented Dec 12, 2022

tomtau left a comment

leejw51crypto commented Dec 12, 2022

leejw51crypto left a comment

leejw51crypto commented Dec 12, 2022

Problem: walletconnnect requests would not be cleared if no response (fix #302) #303

Problem: walletconnnect requests would not be cleared if no response (fix #302) #303

Conversation

damoncro commented Dec 8, 2022 • edited Loading

PR Checklist:

damoncro commented Dec 8, 2022 • edited Loading

tomtau left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 8, 2022 • edited Loading

Codecov Report

leejw51crypto commented Dec 8, 2022 • edited Loading

damoncro commented Dec 8, 2022

damoncro commented Dec 8, 2022 • edited Loading

leejw51crypto commented Dec 8, 2022

leejw51crypto commented Dec 8, 2022

leejw51crypto commented Dec 8, 2022

damoncro commented Dec 9, 2022 • edited Loading

damoncro commented Dec 9, 2022 • edited Loading

leejw51crypto commented Dec 9, 2022 • edited Loading

leejw51crypto commented Dec 9, 2022

damoncro commented Dec 9, 2022

leejw51crypto commented Dec 9, 2022 • edited Loading

damoncro commented Dec 9, 2022

leejw51crypto commented Dec 9, 2022

damoncro commented Dec 9, 2022

leejw51crypto commented Dec 9, 2022

damoncro commented Dec 9, 2022 • edited Loading

damoncro commented Dec 9, 2022

damoncro commented Dec 12, 2022 • edited Loading

tomtau commented Dec 12, 2022

tomtau left a comment

Choose a reason for hiding this comment

leejw51crypto commented Dec 12, 2022

leejw51crypto left a comment

Choose a reason for hiding this comment

leejw51crypto commented Dec 12, 2022

damoncro commented Dec 8, 2022 •

edited

Loading

damoncro commented Dec 8, 2022 •

edited

Loading

codecov-commenter commented Dec 8, 2022 •

edited

Loading

leejw51crypto commented Dec 8, 2022 •

edited

Loading

damoncro commented Dec 8, 2022 •

edited

Loading

damoncro commented Dec 9, 2022 •

edited

Loading

damoncro commented Dec 9, 2022 •

edited

Loading

leejw51crypto commented Dec 9, 2022 •

edited

Loading

leejw51crypto commented Dec 9, 2022 •

edited

Loading

damoncro commented Dec 9, 2022 •

edited

Loading

damoncro commented Dec 12, 2022 •

edited

Loading