-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add http proxy support #110
Conversation
Many corporations require the use of a proxy server for logging etc. - We needed this so I added it. - Add a new 'proxy' option - Move from websockets to aiohttp due to no http proxy support: python-websockets/websockets#364 - Format 'proxy' option into the dictionary httpx requires for 'all://' - Updated requirements / setup.py - Updated CHANGELOG
src/mattermostdriver/websocket.py
Outdated
self._last_msg = time.time() | ||
await event_handler(message) | ||
log.debug("cancelling heartbeat task") | ||
keep_alive.cancel() | ||
try: | ||
await keep_alive | ||
await websocket.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemed with websockets the connection was never cleanly closed. We now do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don’t do this here. The above await always raises CancelledError
Edit: move it below this try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I would use the async context manager so you don’t have to explicitly close the websocket object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do use the context manager so probably don't need this at all ... Will remove after a maintainer reviews to save revisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept it, as it would be nice to cleanly close the websocket then the aiohttp session that the context manager takes care of.
Hey @cooperlees Thanks for this! I merged this already but I am still checking it a bit with mattermost 6.3.0 which it seems somehow cant work with |
Many corporations require the use of a proxy server for logging etc. - We needed this so I added it.