Uh oh!
There was an error while loading. Please reload this page.
Add tornado - #25
Conversation
kinow
commented
Jul 16, 2019
Travis build on Python 2 is failing due to |
bollwyvl
commented
Jul 16, 2019
via email
Sorry to have put this down for so long!
I can certainly rewrite without pathlib... But tornado py2 support is on
the wane, with 6.0 being py3.5+ I believe. …On Tue, Jul 16, 2019, 18:57 Bruno P. Kinoshita ***@***.***> wrote:
Travis build on Python 2 is failing due to pathlib, which is part of
standard lib in Python 3. Should we add pathlib as requirement, or maybe
go with os, path, etc to keep backward compatibility?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#25?email_source=notifications&email_token=AAALCRBITNPQ3TW4ORCM6WLP7ZG6BA5CNFSM4GOI45N2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2CQFVA#issuecomment-512033492>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAALCRGYFUEOIWQNP5CHFTTP7ZG6BANCNFSM4GOI45NQ>
.
|
| @@ -0,0 +1,47 @@ | |||
| from asyncio import Queue | |||
There was a problem hiding this comment.
This is the bigger problem than pathlib for py2 support and would need figuring out...
There was a problem hiding this comment.
Oh, true... maybe https://github.com/tornadoweb/tornado/blob/master/tornado/queues.py?
There was a problem hiding this comment.
I wouldn't worry about py2 support. We'll just call this a py3-only supported backend :)
| @@ -0,0 +1,114 @@ | |||
| from inspect import isawaitable | |||
| from asyncio import ensure_future, wait, shield | |||
There was a problem hiding this comment.
I think Tornado also offers something similar to ensure_future (not sure about wait and shield tho)
bollwyvl
commented
Jul 16, 2019
Yeah, don't know how much I want to wade back into |
kinow
commented
Jul 17, 2019
Yeah.. it would be way easier to update/maintain this if Py3+ only was possible. |
bollwyvl
commented
Jul 17, 2019
Is there stated official cut-off release for Py2? I could certainly hold my breath until one was announced, as I have already vendored it into deathbeds/jupyter-graphql#2, which hasn't been moving much. |
kinow
commented
Jul 26, 2019
Had another look at the changes today, and was thinking in sending a PR to @bollwyvl 's branch to support Py2 as we also want this feature. However, But if the user wants to use websockets or asyncio, then that part of the library is Py3 only? The tests pass on Py2, and I can also install the library with Py2. Running the examples with websockets_lib or asyncio/aiohttp result in error in Py2. So if that's correct, I think we can ignore the Py2 requirement, and use @bollwyvl 's code as-is? The |
bollwyvl
commented
Jul 28, 2019
That's great: no doubt tornado can support py2, and it would not be a big deal to support it, and could likely be written and tested in such a way as to support both tornado 5 and 6, but I am disinclined to advertise support for py2 at this time.
Yeah, that makes it hard to get started as a contributor.
Yeah, I think a pinned issue/README statement of |
dwsutherland
commented
Aug 16, 2019
Ping @syrusakbary (might be busy with wasmerio I suppose..) |
dwsutherland
commented
Sep 3, 2019
@jkimbo - I know you haven't been involved with this one.. But no maintainers appear active here... Do you know what's happening with this project? it seems to be stagnant.. It would be good to have this PR in ... |
| class SubscriptionHandler(websocket.WebSocketHandler): | ||
| def initialize(self, sub_server): | ||
| self.subscription_server = subscription_server |
There was a problem hiding this comment.
I am trying the example locally, but this part was a bit confusing. We have the subscription_server defined before this class declaration.
Here we get self.subscription_server = subscription_server , but we also get a sub_server parameter?
The open() method uses not self.subscription_server , but the previously created subscription_server .
kinow
commented
Sep 5, 2019
Tested the pull request with a small Vue.js application and it worked (thanks @dwsutherland for the help troubleshooting a few minor issues). https://github.com/kinow/tornado-sandbox/tree/master/web5 (copied the |
kinow
commented
May 20, 2020
Ping. We've been happily using the code in this pull request in https://github.com/cylc/cylc-uiserver. Any plans on review/merging it? Thanks! |
SmileyChris
commented
May 20, 2020
Hi @kinow -- I'm new to the team and trying to actively get through the PRs and assess them. I'm currently doing a reshuffle of the core code to split out the common sync and async code across the current servers. Hopefully it should make a smaller effort to integrate, but it may require some rewriting. |
kinow
commented
May 20, 2020
Hi @SmileyChris ! Thanks for the quick reply!
Sounds great!
@bollwyvl 's code is simple well structured, and we have become somewhat familiar with it. So we'd be happy to help testing it with our current system or even trying to suggest how to fix some conflicts if necessary. Thanks! |
kinow
left a comment
There was a problem hiding this comment.
Added a few comments about changes/issues we had after using this code. No blockers, just notes about what we changed 👍 Keeping my +1 for this PR :-)
| await self.queue.put(message) | ||
| async def recv(self): | ||
| return await self.queue.get() |
There was a problem hiding this comment.
In the project where we have been using this code for a while, we ran into issues with the blocking queue here, where some subscriptions would hang, causing memory leaks.
We added a recv_nowait. This way the code calling the queue had a chance to look if the connection was interrupted, and then clear up the remaining asyncio tasks used for the subscription.
| <script src="//cdn.jsdelivr.net/react/15.0.0/react-dom.min.js"></script> | ||
| <script src="//cdn.jsdelivr.net/graphiql/${GRAPHIQL_VERSION}/graphiql.min.js"></script> | ||
| <script src="//unpkg.com/subscriptions-transport-ws@${SUBSCRIPTIONS_TRANSPORT_VERSION}/browser/client.js"></script> | ||
| <script src="//unpkg.com/graphiql-subscriptions-fetcher@0.0.2/browser/client.js"></script> |
There was a problem hiding this comment.
We updated our template recently, using the latest version of graphiql.js, and had issues with the graphiql-subscriptions-fetcher.
Unfortunately the repo for this dependency is archived and hasn't been updated in a while, which causes subscriptions to fail when you update it. So for this PR I think we should leave the current versions, even if old.
Ref: https://github.com/cylc/cylc-ui/pull/457/files#diff-1696770be90f4f901322de74987fdf90R23
SmileyChris
commented
May 27, 2020
#46 is the PR tracking the base class rewrites. You may want to check out that branch to get an idea about building on top of that |
oliver-sanders
commented
Apr 29, 2024
We have been using code derived from this for a while, it works well, thanks all. Happy to review differences and contribute any positive changes once this is in. |
Thanks for all the work!
This adds a tornado handler and context, which is a very minimal change on top of
websockets_lib.py. Tornado is a tad more verbose, I guess, but pretty useful and important in its own right. While theoretically possible, I haven't bothered with python2 support.Because tornado's websocket
on_messageimplementation is callback-based, I've used anasyncio.Queueto make it act more like what other things would expect, but it seems likehandlemight need some more information. I've left it looking for the magicrecv, just to keep the line changes down.I couldn't immediately identify any places to plug in new tests... guess I'll see what CI does!