async datastore in modbus server

[ilkka-ollakka]

Changed datastore to be used by async-flow by modbus server, allowing datastores to be async related to getValues/setValues use.

[janiversen]

Also once the implementation is OK, you need to add test cases, that show the new code works.

[ilkka-ollakka]

I reduced the execute change to be only related the ones that call get/setvalues and server to check if it needs to await the request.

[janiversen]

You are still touching 19 files, far too many !

You did not do, what I asked you to do, to make 2 new methods, you renamed the old ones and that causes the cascade effect.

The server code is not ready for an async datastore, until we have removed the sync parts around it. There are a lot more problems than just making the methods async.

However having async methods allowing the datastore to be accessed outside is not a problem.

[janiversen]

your description says: "Changes codepaths to be async to allow datastore getValues/setValues to be async methods.!"

My suggestion have constantly been, add 2 new methods instead, that will allow you to have async access from outside the server.

You do not write it, but if your purpose is to write a new datastore, you are in for a far bigger job, because the server shares mixin, execute, transaction and framer code with the client both sync and async. So in order to make the datastore async internally, you need to change the server to not share code with any sync part (which includes a couple of examples). This is something that should NOT be done in a single PR, but probably at least 10 PRs

[ilkka-ollakka]

your description says: "Changes codepaths to be async to allow datastore getValues/setValues to be async methods.!"

That was my original intent, to get async datastore to server. Not really to have async access from outside to datastore.

Sorry for causing frustration with my miscommunication on original description and intent related to this PR. I understand that it must have been frustrating.

You do not write it, but if your purpose is to write a new datastore, you are in for a far bigger job, because the server shares mixin, execute, transaction and framer code with the client both sync and async. So in order to make the datastore async internally, you need to change the server to not share code with any sync part (which includes a couple of examples). This is something that should NOT be done in a single PR, but probably at least 10 PRs

From those I think only execute is actually one that blocks calling datastore as async way from server?

To be more splitted, would it be suitable approach for example to change ServerDecoder so that it returns AsyncReadCoilsRequest instead of ReadCoilsRequest etc, and those Async ones would be inherited from the original but have async execute? This way sync flow would be not touched, but it allows to take step closer to async datastore in server and work on that side without needing to touch sync execute-path?

I understand that I might not be enough, so more of mapping on things that are lacking and increasing my own understanding of the scope.

[janiversen]

From those I think only execute is actually one that blocks calling datastore as async way from server?

That is basically correct, but execute is also used in the client code.

To be more splitted, would it be suitable approach for example to change ServerDecoder so that it returns AsyncReadCoilsRequest instead of ReadCoilsRequest etc, and those Async ones would be inherited from the original but have async execute? This way sync flow would be not touched, but it allows to take step closer to async datastore in server and work on that side without needing to touch sync execute-path?

No we do not want to double that amount of Request/Response classes. It is clear that execute should be async when used in asyncio, so maybe a async_execute in a message base could do the trix.....this is just a thought nothing more.

I understand that I might not be enough, so more of mapping on things that are lacking and increasing my own understanding of the scope.

The easiest path, is to look at asyncio.py, because that is the whole server. You basically need to "pursuade" that file to call async_getValues instead of getValues. That is the best way (if possible), second best is to have an async_execute in 1 message base class.

But please no duplication of code and no cascading changes....allowing that is calling for trouble in future maintenance.

I think it is doable, I looked at it in depth about a year ago, when we wanted to make a better forwarder, but got stopped by more urgent matters.

[ilkka-ollakka]

No we do not want to double that amount of Request/Response classes. It is clear that execute should be async when used in asyncio, so maybe a async_execute in a message base could do the trix.....this is just a thought nothing more.

Ok, that sounds good starting point to recheck things.

The easiest path, is to look at asyncio.py, because that is the whole server. You basically need to "pursuade" that file to call async_getValues instead of getValues. That is the best way (if possible), second best is to have an async_execute in 1 message base class.

Currently it does call execute and doesn't know anything about getValues/setValues, but it could be easily changed to call that async_execute. But that would cause change in some Request-classes where there is calls to getValues/setValues.

But please no duplication of code and no cascading changes....allowing that is calling for trouble in future maintenance.

I'll check if I can do something to keep async_execute and execute with minimal code duplication.

[janiversen]

You might need to look from where the execute is called, if it turns out to be only asyncio.py then things are simpler, but I suspect at least some axamples uses the execute.

[janiversen]

A short look at the code, seems to suggest that the execute in the message classes is only used in asyncio.py (it is really a wrong name if that is the case).

If this is the case, the better approach would be to have:

• first PR, that changes the execute in asyncio to be async (hopefully without any side effects)
• second PR, that changes the execute in all message classes to be async (This is a massive change, so it should ONLY contain edits, no functional changes)
• third PR, that changes the datastore.

If you do it all in one PR, it is practically impossible to review, and if/when problems occur down the line, it is very difficult to pin point the cause.

[ilkka-ollakka]

I'll split the things to 4 PR's and link those here.

[ilkka-ollakka]

Intent of this PR is splitted to separated PR's and linked here, so closing this one as it has ran it course