[ISSUE-705] update default value of async validation

[wafa-yah]

Changes included in this PR

After upgrade to v2.0.0 we started having issues in our project (mostly race condition issues), looking into the changelog, it was because of the threading issue raised in #676 by @astrand .
Overriding the ASYNC_VALIDATION added in #678 to False does not help.
Pointing our requirements to the current PR where we changed the default value, brought back the stability to our tests.

Shouldn't the ASYNC_VALIDATION be False by default ? since the case of True is the one known for causing problems with multithreding ?

Current behavior

ISSUE-705

New behavior

• Make ASYNC_VALIDATION False by default.

Impact

• Users that need the ASYNC_VALIDATION to be True need to explicitly override it in there project.
• ⚠️ As mentioned above, the override does not seem to have effect currently, that can be addressed in a separate PR.

Checklist

1. Does your submission pass the existing tests?
2. Are there new tests that cover these additions/changes?
3. Have you linted your code locally before submission?

[wafa-yah]

@astrand @mdwcrft @proelke @jainmohit2001 any input from your side regarding this PR ?

[tmh-grunwald-markus]

Makes sense for me to have this feature deactivated by default. I'm curious what the maintainers will say!

[astrand]

OK with me.

[AlexMagyarosi]

Makes sense to me

[jainmohit2001]

Hi @wafa-yah, the request seems to be reasonable. I want to understand why the override is not working. Can you provide a simple repro?

[jainmohit2001]

@drc38

[drc38]

@drc38

Changing the value in a threaded environment works for me, what's the issue?
ocpp.messages.ASYNC_VALIDATION = False

[jainmohit2001]

@wafa-yah, can you please share more insights into the use case and a repro so that we can find the underlying issue.

[wafa-yah]

@wafa-yah, can you please share more insights into the use case and a repro so that we can find the underlying issue.

Hi @jainmohit2001 sorry for the late reply. In the last few days I'm having issues with our github pipeline's reliability, it's failing randomly and not necessarily because of the tests involving the override. So for now I cannot confirm anymore that the override does not work, it can be that the issue is on our end.

However, I still think that the default value of ASYNC_VALIDATION should be set to False because of the issues raised already in #676. The original lib behavior should remain the default one, and the threading should be the one going under the ASYNC_VALIDATION especially that its introduction is known to bring problems that might be hard to debug for those that are not aware of this flag after upgrading to the latest version.
In addition, I think this flag usage should be highlighted in the lib documentation.

wdt ?

[drc38]

@wafa-yah, can you please share more insights into the use case and a repro so that we can find the underlying issue.

Hi @jainmohit2001 sorry for the late reply. In the last few days I'm having issues with our github pipeline's reliability, it's failing randomly and not necessarily because of the tests involving the override. So for now I cannot confirm anymore that the override does not work, it can be that the issue is on our end.

However, I still think that the default value of ASYNC_VALIDATION should be set to False because of the issues raised already in #676. The original lib behavior should remain the default one, and the threading should be the one going under the ASYNC_VALIDATION especially that its introduction is known to bring problems that might be hard to debug for those that are not aware of this flag after upgrading to the latest version. In addition, I think this flag usage should be highlighted in the lib documentation.

wdt ?

I agree the use of the flag needs more prominence in the documentation, however not on changing its default value as this library is an async library which should not block the event loop when doing I/O operations. Unfortunately due to python limitations (see discussion on the original PR and a workaround for threading) both threading and asyncio do not play nicely together... eg in websockets https://websockets.readthedocs.io/en/stable/ there are two separate implementations.

[wafa-yah]

@wafa-yah, can you please share more insights into the use case and a repro so that we can find the underlying issue.

Hi @jainmohit2001 sorry for the late reply. In the last few days I'm having issues with our github pipeline's reliability, it's failing randomly and not necessarily because of the tests involving the override. So for now I cannot confirm anymore that the override does not work, it can be that the issue is on our end.
However, I still think that the default value of ASYNC_VALIDATION should be set to False because of the issues raised already in #676. The original lib behavior should remain the default one, and the threading should be the one going under the ASYNC_VALIDATION especially that its introduction is known to bring problems that might be hard to debug for those that are not aware of this flag after upgrading to the latest version. In addition, I think this flag usage should be highlighted in the lib documentation.
wdt ?

I agree the use of the flag needs more prominence in the documentation, however not on changing its default value as this library is an async library which should not block the event loop when doing I/O operations. Unfortunately due to python limitations (see discussion on the original PR and a workaround for threading) both threading and asyncio do not play nicely together... eg in websockets https://websockets.readthedocs.io/en/stable/ there are two separate implementations.

But the fact that this is an async python lib is the main argument to not introduce threading by default (async vs multithreding), reasons I see:

• Yes it is unfortunate that python does not handle async and threading together, but:

  • subprocesses and fork() are widely used in unix apps, and quoting python documentation: Even in code that appears to work, it has never been safe to mix threading with os.fork() on POSIX platforms.

  • The multi-threading was not taken into consideration when designing the lib, so this lib is by default not thread-safe , which is sth imposed by the development chosen language and async lib (event loops, tasks, coroutines are all not thread-safe).

  • due to the above reasons, introducing threads should come with:

    • Questioning the choice of the used language itself, as python is NOT thread safe, instead of introducing a known problematic behavior by default to the lib. If the developers using the the lib want to accept that risk, IMHO I think they should explicitly set the flag on their apps and take the consequences into consideration when developing their code.
    • Decent test coverage. In the feature PR, there is only a unit test, I think such serious core design change in the lib needs more testing before accepting that it's safe (which according to python doc, it never can be)

• Introducing this default behavior means also bringing issues by default to all the apps that used this lib already before v2.0.0. Those apps (being python apps) are mostly not thread safe and they didn't need to be as they opted for a lib using async where all code is occurring in the same thread.

• While I understand the need to introduce such a change, I still strongly believe we should adhere to the limitations of the used language, hence not change its default behavior. Especially when it comes to sth where its documentation is clearly stating it can never be safe.

[wafa-yah]

@drc38 @jainmohit2001 any opinions from your side about the change reasons given above ?

[jainmohit2001]

I am open to the thought of reverting the default behavior to be blocking and setting the default value of ASYNC_VALIDATION to False. When we introduced this change, we were aware of the repercussions this might bring and we extensively discussed the limitations of Python language.

To introduce this change, we would have to create another major version.

[wafa-yah]

I am open to the thought of reverting the default behavior to be blocking and setting the default value of ASYNC_VALIDATION to False. When we introduced this change, we were aware of the repercussions this might bring and we extensively discussed the limitations of Python language.

To introduce this change, we would have to create another major version.

Thanks @jainmohit2001 for your answer :)
If it's fine by the maintainers, shall I then merge this PR and and create a major release ?

[jainmohit2001]

@mdwcrft @proelke

[drc38]

@wafa-yah, can you please share more insights into the use case and a repro so that we can find the underlying issue.

Hi @jainmohit2001 sorry for the late reply. In the last few days I'm having issues with our github pipeline's reliability, it's failing randomly and not necessarily because of the tests involving the override. So for now I cannot confirm anymore that the override does not work, it can be that the issue is on our end.
However, I still think that the default value of ASYNC_VALIDATION should be set to False because of the issues raised already in #676. The original lib behavior should remain the default one, and the threading should be the one going under the ASYNC_VALIDATION especially that its introduction is known to bring problems that might be hard to debug for those that are not aware of this flag after upgrading to the latest version. In addition, I think this flag usage should be highlighted in the lib documentation.
wdt ?

I agree the use of the flag needs more prominence in the documentation, however not on changing its default value as this library is an async library which should not block the event loop when doing I/O operations. Unfortunately due to python limitations (see discussion on the original PR and a workaround for threading) both threading and asyncio do not play nicely together... eg in websockets https://websockets.readthedocs.io/en/stable/ there are two separate implementations.

But the fact that this is an async python lib is the main argument to not introduce threading by default (async vs multithreding), reasons I see:

• Yes it is unfortunate that python does not handle async and threading together, but:

  • subprocesses and fork() are widely used in unix apps, and quoting python documentation: Even in code that appears to work, it has never been safe to mix threading with os.fork() on POSIX platforms.

  • The multi-threading was not taken into consideration when designing the lib, so this lib is by default not thread-safe , which is sth imposed by the development chosen language and async lib (event loops, tasks, coroutines are all not thread-safe).

  • due to the above reasons, introducing threads should come with:

    • Questioning the choice of the used language itself, as python is NOT thread safe, instead of introducing a known problematic behavior by default to the lib. If the developers using the the lib want to accept that risk, IMHO I think they should explicitly set the flag on their apps and take the consequences into consideration when developing their code.
    • Decent test coverage. In the feature PR, there is only a unit test, I think such serious core design change in the lib needs more testing before accepting that it's safe (which according to python doc, it never can be)

• Introducing this default behavior means also bringing issues by default to all the apps that used this lib already before v2.0.0. Those apps (being python apps) are mostly not thread safe and they didn't need to be as they opted for a lib using async where all code is occurring in the same thread.

• While I understand the need to introduce such a change, I still strongly believe we should adhere to the limitations of the used language, hence not change its default behavior. Especially when it comes to sth where its documentation is clearly stating it can never be safe.

I should have been clearer above, mixing threading and asyncio is fine and expected in Python https://docs.python.org/3/library/asyncio-dev.html. It is os.fork() that is the issue with threading and hence asyncio programming. Here's the link from the original discussion by the Python devs on why os.fork() needs to be deprecated for spawn by developers where there is the possibility of multithreading:
https://discuss.python.org/t/concerns-regarding-deprecation-of-fork-with-alive-threads/33555/20