AbstractSDK / cw-orchestrator

All-in-one Rust-based CosmWasm testing, scripting, and deployment tool.
https://orchestrator.abstract.money
GNU General Public License v3.0
76 stars 18 forks source link

Daemon state lock #365

Closed Kayanski closed 4 months ago

Kayanski commented 5 months ago

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

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

Kayanski commented 5 months ago

This is a simpler modification compared to https://github.com/AbstractSDK/cw-orchestrator/pull/326

Kayanski commented 5 months ago

@CyberHoward @Buckram123, thoughts ?

cloudflare-pages[bot] commented 5 months ago

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

codecov[bot] commented 5 months ago

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](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/365?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK) | Coverage Δ | | |---|---|---| | [cw-orch-daemon/src/state.rs](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/365?src=pr&el=tree&filepath=cw-orch-daemon%2Fsrc%2Fstate.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK#diff-Y3ctb3JjaC1kYWVtb24vc3JjL3N0YXRlLnJz) | `89.0% <100.0%> (+0.1%)` | :arrow_up: | | [cw-orch-daemon/src/json\_file.rs](https://app.codecov.io/gh/AbstractSDK/cw-orchestrator/pull/365?src=pr&el=tree&filepath=cw-orch-daemon%2Fsrc%2Fjson_file.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AbstractSDK#diff-Y3ctb3JjaC1kYWVtb24vc3JjL2pzb25fZmlsZS5ycw==) | `96.9% <94.8%> (-3.1%)` | :arrow_down: |
Buckram123 commented 5 months ago

@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:

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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

The implementation in https://github.com/AbstractSDK/cw-orchestrator/pull/326 was favored