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

[r2r] Fix task::*::cancel if the RPC task is an awaiting status #1582

Merged
merged 1 commit into from Dec 26, 2022

Conversation

sergeyboyko0791
Copy link

@sergeyboyko0791 sergeyboyko0791 commented Dec 20, 2022

The issue was:

  1. cancel_init_trezor:
    https://github.com/KomodoPlatform/atomicDEX-API/blob/6c90c28a29723dad6c48296bed3093686188eeb2/mm2src/mm2_main/src/lp_init/init_hw.rs#L209
  2. RpcTaskManager::cancel_task:
    https://github.com/KomodoPlatform/atomicDEX-API/blob/6c90c28a29723dad6c48296bed3093686188eeb2/mm2src/rpc_task/src/manager.rs#L111-L112
  3. A corresponding item is removed from the RpcTaskManager::tasks:
    https://github.com/KomodoPlatform/atomicDEX-API/blob/6c90c28a29723dad6c48296bed3093686188eeb2/mm2src/rpc_task/src/manager.rs#L21-L22
  4. If the task is in Awaiting status, abort_handle AND action_sender are dropped
    https://github.com/KomodoPlatform/atomicDEX-API/blob/6c90c28a29723dad6c48296bed3093686188eeb2/mm2src/rpc_task/src/manager.rs#L269-L274
  5. I don't know why, but action_sender channel is closed before abort_handle, so the task finishes with an error, but we expect that it should abort.

@sergeyboyko0791 sergeyboyko0791 added this to Todo in MM 2.0 via automation Dec 20, 2022
@sergeyboyko0791 sergeyboyko0791 moved this from Todo to PR review in MM 2.0 Dec 20, 2022
@shamardy
Copy link
Collaborator

I don't know why, but action_sender channel is closed before abort_handle, so the task finishes with an error, but we expect that it should abort.

Do you think the drop order can affect this? If we reversed action_sender and abort_handle positions, would that solve the problem. Just curious :)

@sergeyboyko0791
Copy link
Author

I haven't checked, but this behaviour is dependent on the executor, and we can't guarantee that the abort_handle receiver will be notified at first, and the action_sender later.
So I decided to make the 100% solution :)

Do you think the drop order can affect this? If we reversed action_sender and abort_handle positions, would that solve the problem. Just curious :)

@sergeyboyko0791
Copy link
Author

sergeyboyko0791 commented Dec 21, 2022

@shamardy I've checked - the order doesn't matter 😁
I.e. it matters, but not so much that the order of awaken futures depends on it

@shamardy
Copy link
Collaborator

I've checked - the order doesn't matter 😁

I thought that it won't, I was just curious as I said :)

So I decided to make the 100% solution :)

Agreed, it's better than leaving it in the hands of a race condition.

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

LGTM!

@smk762
Copy link

smk762 commented Dec 21, 2022

Just gave it a test in CLI.
PIN grid stays on screen when cancelling the init task, but I can init again and the grid changes each time I cancel and re-init.

@sergeyboyko0791
Copy link
Author

As expected. It's turned out to be difficult to fix, so I created the issue #1586

Just gave it a test in CLI. PIN grid stays on screen when cancelling the init task.

Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

👍

@artemii235 artemii235 merged commit 4f1190b into dev Dec 26, 2022
MM 2.0 automation moved this from PR review to Done Dec 26, 2022
@artemii235 artemii235 deleted the cancel-rpc-task-fix branch December 26, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MM 2.0
  
Done
Development

Successfully merging this pull request may close these issues.

task::*::cancel doesn't work properly if the RPC task is in an awaiting status.
4 participants