-
Notifications
You must be signed in to change notification settings - Fork 50
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
Conversation
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 |
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. |
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
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 |
98f799d
to
8181586
Compare
Thanks @ChaoticTempest, I've implemented what you wanted |
There was a problem hiding this 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
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.