Create a new feature that allows you to use a full async-client

[svanharmelen]

I haven't tested enough with this new client to make sure all blocking code is now properly async, but I'm definitely close.

[svanharmelen]

@locka99 very interested to know how you think about a PR like this. Are you open to adding something like this to the crate? The only downside I see is that fixes to one client cannot be immediately applied to the other. That requires some manual syncing which could be painful. Yet I'm very open to take responsibility to keep the async client inline with any fixes applied to the default client...

[ctron]

I would really like to see this async client become available!

One thing I noticed: the callbacks are currently still sync. While I think it is ok to keep them. I would also love to see some async callbacks. I guess async_trait could be leveraged to make this a bit simpler.

[sysarcher]

Hi Folks,

One question on the goals of an async client. If there is need for determinism (cpu-pinning + cyclic-time guarantees, maybe also cache allocation), will this cyclic client be the right choice still? Are there knobs in the design to enable such things in the future?

Thanks.

[ctron]

Hi Folks,

One question on the goals of an async client. If there is need for determinism (cpu-pinning + cyclic-time guarantees, maybe also cache allocation), will this cyclic client be the right choice still? Are there knobs in the design to enable such things in the future?

Thanks.

As this is based on Tokio, I guess you will get all that Tokio gives you.

[lovasoa]

Looking at the code, it seems that there is a lot of code duplication between the sync and the async client... There are currently multiple bugs in the sync client, and this means they will now have to be fixed twice... If we want to keep backwards compatibility, then shouldn't we use the async client from the sync client, probably with a lot of block_on ?

[ctron]

If we want to keep backwards compatibility, then shouldn't we use the async client from the sync client, probably with a lot of block_on ?

I guess that makes sense. However, I also think that a two phase approach might be useful. 1) create an async client that works, and don't change the existing sync client 2) replace the sync client with an implementation of the async using, using block_on as you suggested.

It might be easier to just start from scratch with the async one, and don't be tied down by existing APIs or implementations.

Just my two cents …

[svanharmelen]

I don't think using block_on all over the place will result in a true async client that can be used with applications that are already build upon Tokio and already start their one runtime...

I did create this client starting with a full copy of the sync client with the intent that it gives us (me) a way to rebase on the sync client every now and then. Will try to do that later today/tomorrow to see how that works out.

[lovasoa]

I don't think using block_on all over the place will result in a true async client that can be used with applications that are already build upon Tokio and already start their one runtime...

I think there is a misunderstanding here. I was talking about having the old sync client api being implemented based on the new async one. This way existing code that uses the sync API keeps working, but there is no code duplication between the sync and the async client.

[lovasoa]

I did create this client starting with a full copy of the sync client with the intent that it gives us (me) a way to rebase on the sync client every now and then. Will try to do that later today/tomorrow to see how that works out.

Don't you think rebasing after every change to either clients is going to be a lot of work ? And that people probably won't want to go through the trouble of fixing issues in one of the clients if they only use the other ? I can see a multiple of the bugs and performance issues that I fixed in #201 reproduced in the async client introduced here.

[lovasoa]

Another thing is: a lot of the client's code is only tested in integration tests. Should they be rewritten to use the async client ?

[svanharmelen]

I think there is a misunderstanding here. I was talking about having the old sync client api being implemented based on the new async one. This way existing code that uses the sync API keeps working, but there is no code duplication between the sync and the async client.

I get it... So the existing sync client would be sort of a wrapper around the async client. Guess that would make sense...

Don't you think rebasing after every change to either clients is going to be a lot of work ? And that people probably won't want to go through the trouble of fixing issues in one of the clients if they only use the other ?

Of course this is not be a permanent solution or way of working and I don't expect anyone else (besides me) to do that until the async client is actually fully ready. And by the time it is fully ready, I guess we need a good chat with @locka99 to see how to proceed in a more structured way. As indeed this way of working only makes sense while the async client is still in development.

I can see a multiple of the bugs and performance issues that I fixed in https://github.com/locka99/opcua/pull/201 reproduced in the async client introduced here.

Yes, because I didn't rebase since I created the draft PR...

Another thing is: a lot of the client's code is only tested in integration tests. Should they be rewritten to use the async client ?

Guess that depends on what setup we end up with. If the sync client is going to be a wrapper around the async client, then I think yes. But that is to early to say now as I have no clue how @locka99 sees all this.

I just know I needed a fully async client that could work with an existing Tokio runtime, so I started this work to test if I could make that happen. But I didn't have any discussions on how (if at all) to integrate this in the existing project.

[locka99]

I haven't commented yet but when it comes to async, are you wanting async in the futures / tokio sense where you have async / await style functionality, or async in the sense that you call a function and you get a callback later on it returning?

If the former, then the api should probably just resemble the existing one with the functions being async. i.e. you just call it much the way it works now but you await it coming back. I don't know how this would work with all the locks we have all over the client but ideally they'd all become async too.

Functions would have to timeout the way they do now. The existing publish functionality would have to be reworked to fit in with the new way of doing things. Ideally the sync client would remain as-is since I don't think clients benefit as much from async as the server side would.

[svanharmelen]

If the former, then the api should probably just resemble the existing one with the functions being async. i.e. you just call it much the way it works now but you await it coming back

This is exactly what I was trying to do. If you look at the 3rd commit you can see the changes I made with respect to the existing sync client: https://github.com/locka99/opcua/pull/197/commits/445ef6f13797de5dbdbba89aaae89afde535f399

As you can see, I mainly converted specific functions so they become async functions which the caller has to await by only adjusting the function declaration. And only there where needed anything that blocks a thread is converted to an async alternative.

I don't know how this would work with all the locks we have all over the client but ideally they'd all become async too.

Indeed the locks are the biggest pain here and I expect they are the cause of the last issue I'm seeing (the client sometimes hanging after running fine for a random time). Still looking into how to properly fix that, but sometimes work prevents me from spending to much time on this. But its still on my radar!

[ctron]

Indeed the locks are the biggest pain here and I expect they are the cause of the last issue I'm seeing (the client sometimes hanging after running fine for a random time).

I those are standard locks, they might actually lock up the I/O threads of Tokio. There is a guide from Tokio regarding "shared state": https://tokio.rs/tokio/tutorial/shared-state

I guess in case the second approach (spawning a task and doing some messaging passing) might be the easiest. I am not sure replicating the existing (sync) implementation makes the implementation easier and more stable.

[lovasoa]

I am not sure replicating the existing (sync) implementation makes the implementation easier and more stable.

Indeed, but rewriting a new client from scratch based on a better design is also much more work. I think the path started here is reasonable. First duplicate the existing client and make it async just by "blindly" replacing blocking functions with async equivalents. Then replace the existing blocking client by wrappers around the new async one. Then progressively rewrite components in the new async clients, eliminating global mutable state one variable at a time.

This way backward compatibility is preserved, the code can still be used and maintained the entire time, and the effort can be shared between multiple contributors.

[ctron]

I think the path started here is reasonable.

Absolutely! And I didn't want to discourage you from doing so!

[milgner]

Since a lot of the discussion here is about re-using an existing Tokio runtime: I recently found that using Handle::current() is a good way to access a pre-existing runtime to block on futures where a sync API is desired. Imho it would be okay to just ask users who don't want an asynchronous interface to instantiate a runtime themselves and then this crate would be able to provide a sync layer over the async one without having to instantiate a runtime. But at least one wouldn't have to see the dreaded Cannot drop a runtime in a context where blocking is not allowed. any more :sweat_smile:

[svanharmelen]

I'm going to close this one for now. Maybe I'll reopen it with an updated version in the future, but for now (since it's almost a year old and not being updated anyway) I guess it makes sense to just close it 👍🏻