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

Daemon state lock #365

Closed
wants to merge 2 commits into from
Closed

Daemon state lock #365

wants to merge 2 commits into from

Conversation

Kayanski
Copy link
Contributor

@Kayanski Kayanski commented Apr 9, 2024

This PR aims at avoiding simultaneous write/read conflicts between instances of cw-orch.
It uses:

  • file-lock crate to avoid inter-processes conflicts
  • fs4 crate to avoid intra-processes conflicts

fs4 needs a manual unlock
file-lock is automatically unlocked when FileLock variable gets out of scope

@Kayanski
Copy link
Contributor Author

Kayanski commented Apr 9, 2024

This is a simpler modification compared to #326

@Kayanski
Copy link
Contributor Author

Kayanski commented Apr 9, 2024

@CyberHoward @Buckram123, thoughts ?

Copy link

Deploying cw-orchestrator with  Cloudflare Pages  Cloudflare Pages

Latest commit: ab59a08
Status: ✅  Deploy successful!
Preview URL: https://704de667.cw-orchestrator.pages.dev
Branch Preview URL: https://test-daemon-state-lock.cw-orchestrator.pages.dev

View logs

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 64.4%. Comparing base (845707c) to head (ab59a08).

Additional details and impacted files
Files Coverage Δ
cw-orch-daemon/src/state.rs 89.0% <100.0%> (+0.1%) ⬆️
cw-orch-daemon/src/json_file.rs 96.9% <94.8%> (-3.1%) ⬇️

@Buckram123
Copy link
Contributor

Buckram123 commented Apr 9, 2024

@CyberHoward @Buckram123, thoughts ?

It is much simpler and it will work for the most of the cases! What I'm not sure about is that file can get unlocked in the middle of script execution, and value overwritten by another program that uses this state file.

Let's imagine an example where we have 2 scripts both of them use cw20 contract, but with different addresses:

  • First script uploads cw20 contract and saves it's address under cw20 key in state.json
  • Second script uploads cw20 contract and saves it's address under cw20 key in state.json

Now we have a situation where first script uses different contract, than it expects to be using. It is an extreme edge case, but that's why locking it for the whole state file usage was one of the goal of #326

@Kayanski
Copy link
Contributor Author

Kayanski commented Apr 9, 2024

@CyberHoward @Buckram123, thoughts ?

It is much simpler and it will work for the most of the cases! What I'm not sure about is that file can get unlocked in the middle of script execution, and value overwritten by another program that uses this state file.

Let's imagine an example where we have 2 scripts both of them use cw20 contract, but with different addresses:

* First script uploads cw20 contract and saves it's address under `cw20` key in `state.json`

* Second script uploads cw20 contract and saves it's address under `cw20` key in `state.json`

Now we have a situation where first script uses different contract, than it expects to be using. It is an extreme edge case, but that's why locking it for the whole state file usage was one of the goal of #326

Wait. That's normal, if a script modifies something in the state file and another one modifies it in another way, what you are saying is expected behavior ! We can't lock the state file for the entirety of the usage ! If the value needs to be overwritten by another program, then it should be overwritten I think

@Buckram123
Copy link
Contributor

Wait. That's normal, if a script modifies something in the state file and another one modifies it in another way, what you are saying is expected behavior ! We can't lock the state file for the entirety of the usage ! If the value needs to be overwritten by another program, then it should be overwritten I think

It's not normal since developer shouldn't expect other script to be messing with his state file while he's running the script

@CyberHoward
Copy link
Contributor

CyberHoward commented Apr 10, 2024

The choices here are really about safety VS enabling multi-threaded use.

With Misha's solution you can have multiple processes compete for access to the state file but only one process will have access to it at any time. Handover of that access happens when the owner's process finalized. Then another process acquires the file and proceeds.

What Nicolas is trying to do here is to enable multiple processes to access the file during their execution. I.e. the process doesn't lock the file for the whole duration of the execution but only when writing to it.

I think that gradually enabling multithreading in this way will lead to more bugs than we expect (also think account sequence). So I tend to lean into Misha's approach to it more, unless we're able to address this multithreading feature as a whole. I.e. make a doc on what needs to change for us to be able to multithread Daemons, and then take it from there.

On the flipside, if we merge Misha's implementation then there's quite some code that we'll need to remove later if we want to go to a multithreaded solution.

PS: tell me if I misunderstood anything

@Kayanski
Copy link
Contributor Author

The implementation in #326 was favored

@Kayanski Kayanski closed this Apr 23, 2024
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