PLAT-10279 Fix async calls not using truststorePath #87
Conversation
anthony-symphony
force-pushed
the
truststore-fix
branch
from
770085a to
4885900
Compare
There was a problem hiding this comment.
Thanks for the PR @anthony-symphony!
We will try to reproduce it on our end too. Maybe it is worth checking if the latest version of aiohttp still has the windows issue
| @@ -51,6 +54,16 @@ def __init__(self, auth, config): | |||
| self.async_agent_session = None | |||
| self.bot_user_info = None | |||
| self.health_check_client = None | |||
| #Required for aiohttp to use truststore | |||
There was a problem hiding this comment.
the downside here is that we are creating the ssl context even if we are not using the async calls
but at the same time, code is factored in one place and we avoid using the session object to pass the ssl context around
There was a problem hiding this comment.
Maybe we could extract that to a method get_async_ssl_context called only where it is needed. It would create the async SSL context the first time and reuse it next times,
| logging.debug("Setting truststore for async calls to system truststore") | ||
| self.async_ssl_context = ssl.create_default_context() | ||
| #Required to work around a bug in Python 3.8+ on Windows. Can be removed once fixed in aiohttp | ||
| if sys.version_info[0] == 3 and sys.version_info[1] >= 8 and sys.platform.startswith('win'): |
There was a problem hiding this comment.
is there a risk that we are changing the loop policy too late? like if asyncio has already been initialized before?
I have mixed feelings on adding this in the BDK, maybe it would be safer to use it as a workaround (in the bot) when you are facing the issue?
I'm thinking for instance what if in some context setting this loop policy is the wrong thing to do?
There was a problem hiding this comment.
I am open to taking that out and putting it in the README.md
There was a problem hiding this comment.
@anthony-symphony Yes please I would be more comfortable with this specific workaround out of the code base
Do our customers use Windows for production or only for development?
Created get_async_ssl_context to set it when called instead of setting it in init Added note with workaround for Windows users on Python 3.8+
* PLAT-10139: Added missing event handlers in AsyncDatafeedEventService (#80) * PLAT-10225: Fix datafeed loop crash for events without id (#81) * PLAT-10225: Fix datafeed loop crash for events without id A datafeed event does not always have an id (for instance when external users join a room) therefore we must handle that. The access to the id is replaced with .get() to avoid the runtime error and if the id is missing a random one is set for tracing purpose. Added a unit test for that using IsolatedAsyncioTestCase, it is a bit complex due to the async nature of the code but it does run part of the datafeed loop. * Add Mac files to .gitignore * Fix import in examples * Run pylint on test * Use GH release page instead of release note (#82) It will simplify the releases, no need to maintain changelog * PLAT-10228 Prepare 1.3.1 release (#83) * PLAT-10228 Prepare 1.3.1 release Update version Automate with a release script and Docker Add to set a specific version of ecdsa for compatibility with python-jose * Use fixed and smaller image * Docker image for release does not have bash installed (#85) Switch back to Debian based image to use bash * PLAT-10228: Use Jenkins credentials in release job (#86) The credentials are selected via job parameters and passed as environment variables * PLAT-10279 Fix async calls not using truststorePath (#87) * Fixed async datafeed not using truststorePath * Remove aiohttp workaround for Windows Created get_async_ssl_context to set it when called instead of setting it in init Added note with workaround for Windows users on Python 3.8+ * PLAT-10245: Support CircleCI build for master branch (#95) In the 2.0 branch we introduced CircleCI and PR builders, just to make it work with the master branch, here we add a simple build file just running the tests (the bare minimum, the 2.0 branch supports Snyk, test coverage and reports). 3.6 cannot be used because we use unittest.async_case. Caching with pyenv was not working (dependencies problem) so it is not implemented (we do not expect a lot of executions). * Handle asyncio.CancelledError (#88) * Handle asyncio.CancelledError * Moved where CancelledError Exception is handled Needed for 3.8+ compatbility as CancelledError was changed from an Exception to a BaseException Co-authored-by: Youri Bonnaffe <youri.bonnaffe@symphony.com> * PLAT-10373: Bump version to 1.3.2 Co-authored-by: Elias Croze <66254040+symphony-elias@users.noreply.github.com> Co-authored-by: anthony-symphony <46792802+anthony-symphony@users.noreply.github.com>
Ticket
PLAT-10279
Description
Datafeed and other async calls that used aiohttp did not use the defined truststore in the config.json
Dependencies
No additional dependencies