Skip to content
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

WebsocketHandler.get is async #119

Merged
merged 4 commits into from
Apr 8, 2019
Merged

Conversation

rmorshea
Copy link
Contributor

@rmorshea rmorshea commented Apr 6, 2019

As of Tornado v6.0.2 some methods were converted to be async/await capable. See commit:

tornadoweb/tornado@e69becb

Closes: #39

As of Tornado v6.0.2 some methods were converted to be async/await capable. See commit:

tornadoweb/tornado@e69becb
@betatim
Copy link
Member

betatim commented Apr 6, 2019

How could we deal with being compatible with Tornado 5 and 6 at the same time?

I was thinking https://www.tornadoweb.org/en/stable/gen.html#tornado.gen.maybe_future but that is deprecated. So maybe https://docs.python.org/3/library/inspect.html#inspect.isawaitable is the right thing to use if we should await or not?

@rmorshea
Copy link
Contributor Author

rmorshea commented Apr 6, 2019

Got it. Will make that change tomorrow.

Check whether or not the result of `WebSocketHandler.get` is awaitable or not before awaiting.
@rmorshea
Copy link
Contributor Author

rmorshea commented Apr 6, 2019

@betatim done!

@ian-r-rose
Copy link
Collaborator

Recently the notebook server swapped out tornado's maybe_future for its own to deal with that deprecation: jupyter/notebook#4453 . That may be a good choice here as well.

@rmorshea
Copy link
Contributor Author

rmorshea commented Apr 6, 2019

@ian-r-rose does this problem show up in other places in this repo? I think it makes sense to use that if there's more locations where we need to address the backward compatibility.

@ian-r-rose
Copy link
Collaborator

@rmorshea I'm not sure. This seems to fix the immediate problem, but there may be more lurking somewhere. I confess I don't have the best grasp of websockets.

@rmorshea
Copy link
Contributor Author

rmorshea commented Apr 6, 2019

@ian-r-rose neither do I. @betatim actually debugged #39 I just made the PR.

@betatim
Copy link
Member

betatim commented Apr 6, 2019

I don't really know either. Having looked at the implementation of maybe_future in the notebook I am now thinking that we should use that. As with all these things it isn't as easy as just asking if isawaitable(obj). Especially as it seems that in this thread it is mostly the blind leading the blind 👀.

Does this mean that jupyter-server-proxy will require a brand new notebook package in order to work (if we use maybe_future from there)? If so that is also not ideal.

@ian-r-rose
Copy link
Collaborator

@betatim That's a good point. Perhaps we should try to import maybe_future, then fall back on gen.maybe_future if that fails?

@rmorshea
Copy link
Contributor Author

rmorshea commented Apr 6, 2019

@betatim used a try/except to import from notebook and then from tornado.

rmorshea added a commit to reactive-python/reactpy that referenced this pull request Apr 7, 2019
Binder was not working due to a websocket problem with the
Jupyter proxy server. See the patch here:

jupyterhub/jupyter-server-proxy#119

Also added documentation for the examples and added links to
binder from the README and docs.
rmorshea added a commit to reactive-python/reactpy that referenced this pull request Apr 7, 2019
Binder was not working due to a websocket problem with the
Jupyter proxy server. See the patch here:

jupyterhub/jupyter-server-proxy#119

Also added documentation for the examples and added links to
binder from the README and docs.
rmorshea added a commit to reactive-python/reactpy that referenced this pull request Apr 7, 2019
* add support for binder

* try custom websocket patch

* Get binder working + docs

Binder was not working due to a websocket problem with the
Jupyter proxy server. See the patch here:

jupyterhub/jupyter-server-proxy#119

Also added documentation for the examples and added links to
binder from the README and docs.

* be sure users click the image

* update lock file
@betatim betatim merged commit 7ac0125 into jupyterhub:master Apr 8, 2019
@betatim
Copy link
Member

betatim commented Apr 8, 2019

Merged. Thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants