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

Removed the lifetime in transact_async #249

Merged
merged 7 commits into from
Jan 9, 2023
Merged

Conversation

DavidM-D
Copy link
Contributor

@DavidM-D DavidM-D commented Dec 2, 2022

Addresses #248

There are probably further improvements I could make to the API of workspace now that Client has a clone instance, but I wanted to start with a PR with a small footprint and get feedback.

@ChaoticTempest
Copy link
Member

Cloning the client looks decent, but there is one thing that can go wrong. If we have a transaction_status in a thread/task that outlives the worker, then we run into a footgun where sending the transaction would error out since the network is down. This is mainly on the sandbox network, since worker manages it; so if it gets dropped, the sandbox environment gets dropped. The lifetime ensured this requirement before, so to circumvent this problem, it's better to pass down a clone of the worker instead of cloning the client, and storing the worker in the TransactionStatus object.

@DavidM-D
Copy link
Contributor Author

DavidM-D commented Dec 9, 2022

So I stuck a counted reference to the sandbox in the client to guarantee that while there's still a client about the server won't be dropped. It seems a little bit less safe that the previous implementation, you can probably accidentally keep a sandbox around when you'd rather not, but it seems a little more ergonomic.

How do you feel about the tradeoff @ChaoticTempest?

Furthermore, if you're happy I think I can strip out most of the lifetimes in the library.

@ChaoticTempest
Copy link
Member

That looks like a hack though since now we have client managing a sandbox server which the worker already does. The client shouldn't worry about managing network specific details like these, since that's the job of the worker. So Client should be agnostic of networks, meanwhile Worker knows about which network. You can pass down a Worker<dyn Network> to achieve the dynamic network which can be stored directly in TransactionStatus without all this mixing of network details into client.

Furthermore, if you're happy I think I can strip out most of the lifetimes in the library.

we can worry about this in a future issue if someone else has issues, since the lifetimes help with telling that certain operations are constrained and shouldn't necessarily be stored

@DavidM-D
Copy link
Contributor Author

Thanks @ChaoticTempest, I've implemented what you wanted

Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

LGTM. Can you add a CHANGELOG entry before merging

@DavidM-D DavidM-D merged commit 17e402a into main Jan 9, 2023
@DavidM-D DavidM-D deleted the dmd/easy_transact_async branch January 9, 2023 13:54
@frol frol mentioned this pull request Oct 4, 2023
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.

2 participants