Closed einarmo closed 2 months ago
Hi thanks a million for this submission. It'll take me some time to go through it so have some patience and pester me if I don't come back in a week or so.
Of course. Thank you for looking at it. Feel free to reach out if there's anything.
Hi Einar, I think the patch looks good and I really appreciate what must have been a substantial effort. There is a lot to it but I ran through some basic scenarios against OPCUA servers which passed so I am happy to accept the patch.
Wow, I did not expect this to go so quickly. Thank you for the trust and for accepting the patch. I do have some more stuff coming eventually, though hopefully nothing quite so extensive.
I hope to expand a great deal on the integration tests as well, if I find the time.
Thanks for your contribution, @einarmo
I've been using this library for a service I've been working on (for production) and have run into some nits along the way
Really encouraging to see others that want to improve this library
Curious if a server rewrite would be a good time to look at #281 or #285
Heck, even the rewrite alone might resolve the issues 🤞
@evanjs if I do get around to the server (I've started looking a bit, but it will likely be quite a while before I have anything substantial), then I would be very surprised if I didn't somehow fix these two issues.
This is a complete rewrite of the client, in order to make it async the entire way through, removing the need for any internal tokio runtimes.
First of all, sorry about this. It's never nice to receive such a large PR out of nowhere. Consider this an announcement that this exists. I hope we can work together to figure out a way to get this into the library in some way, as I consider the changes made here to be positive.
No matter where this PR goes I will likely keep working on the things outlined in the "Future projects" section of
async_client.md
.See docs/async_client.md for an overview of the technical details on this change. Very briefly: the client has been rewritten from the ground up to be entirely async. It is now possible to run a session without spawning a single thread.
Background
I have some background with OPC-UA, mostly in .NET, and I've fallen for rust. I consider rust a natural fit for OPC-UA. The focus on correctness is helpful when working with such a huge complicated standard, and it should be possible to make an OPC-UA client/server implementation very lightweight.
For a project, I need a lightweight OPC-UA client. In particular, I want it to be so lightweight that I can run thousands of clients without issue. This means that a client should have almost no overhead, and I need it to be async so that I can run them cooperatively. This is essentially impossible in any other OPC-UA framework I know of.
I first considered making my own OPC-UA library, but that idea was quickly discarded as it would be a waste of time to redo the significant amount of work that goes into cryptography, binary encoding/decoding, and other core utilities already implemented and tested here.
Initially, this was supposed to just change the existing client implementation to async, step by step, but it quickly became clear that a lot of changes needed to be done in order to make that happen, and an intermediate state would be very hard to create, at least without a final goal to reference, so I decided to start fresh. Despite what the diff line count would have you believe, this does reuse a lot of code from the old client.
Scope
The only limitation on this project was that I wanted to avoid touching anything outside of the
client
itself. Of course, samples and tests also had to be updated, and a few utilities were changed for technical reasons, seeasync_client.md
for details on that too.Tests
A significant change to working with the library is that the tests can now be run normally, using just
cargo test
, it takes about 15 seconds on my machine, which is pretty good. The downside is that the need to usespawn_blocking
in the tests makes them handle panics very poorly. Dealing with this is going to require changing the server to be async as well (which I do think should be done).