Terminal broken on master of notebook and terminado

[ellisonbg]

When just did a clean install from master of notebook and terminado and the terminal is broken. I see the following traceback in the log file:

    Traceback (most recent call last):
      File "/usr/local/lib/python3.4/dist-packages/tornado/websocket.py", line 417, in _run_callback
        callback(*args, **kwargs)
      File "/usr/local/lib/python3.4/dist-packages/notebook/base/zmqhandlers.py", line 179, in open
        return super(WebSocketMixin, self).open(*args, **kwargs)
      File "/tmp/src/terminado/terminado/websocket.py", line 90, in open
        if not self.check_origin():
    TypeError: check_origin() missing 1 required positional argument: 'origin'

[ellisonbg]

I am not sure what the fix is, but I see roughly where the problem is:

• In tornado>=4 there is a WebSocketHandler.check_origin(self, origin) method that checks the origin of web sockets. It has a required positional argument.
• In jupyter/notebook, we have a subclass that we use in our handlers ZMQStreamHandler(WebSocketMixin, WebSocketHandler) which also has the check_origin method with the required positional.
• In terminado there is TermSocket(tornado.websocket.WebSocketHandler) which has a check_origin method with an optional keyword argument.
• In TermSocket.open self.check_origin() is called with no arguments.
• At first glance this looks like it should work fine because it will be calling terminado's subclasses check_origin.
• But, the code which calls open in juipyter/notebook does so by doing super(WebSocketMixin, self).open(*args, **kwargs).
• Somehow, that is causing the base tornado check_origin to be called, but with no arguments, which results in an exception.

The part that I am still confused about is that WebSocketMixin doesn't have the base tornado WebSocketHandler in its mro, so I don't see how it is resolving to that.

Questions:

• All of this logic in terminado is to handle tornado<4. But tornado 3 is really old now and jupyter/notebook now requires tornado>=4. Can we just remove all this logic from terminado?
• Is there something more subtle going on with the inheritance here that needs to be fixed?
• The tornado check_origin method has never had a different signature. Buy terminado subclassing and changing the signature, this was bound to happen at some point. If we keep the logic in terminado, can we address this API difference?

@minrk @takluyver

[ellisonbg]

Also, I think we should release a new version of terminado along with 4.1 of the notebook. There were other things fixed in terminado recently that all users should be using.

[fperez]

I'm curious, I'm not seeing this problem on my side... Wondering why, is it a difference in terminado versions? I'm running master of the notebook, terminado 0.5, tornado 4.3.

[ellisonbg]

I think it was these changes to terminado in October that are related to it:

jupyter/terminado@2e362b0

I am running from master as there are some other recent changes that fix
another bug in terminado.

Can you install terminado from master and see if you can reproduce?

On Wed, Dec 30, 2015 at 6:22 PM, Fernando Perez [email protected]
wrote:

I'm curious, I'm not seeing this problem on my side... Wondering why, is
it a difference in terminado versions? I'm running master of the notebook,
terminado 0.5, tornado 4.3.

—
Reply to this email directly or view it on GitHub
#912 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[fperez]

Yup, just installed terminado master, and I now see the same exact traceback you're reporting.

So at least I can confirm the problem...

[fperez]

The symptom is then that the terminal never initializes, so it's pretty much hosed.

[fperez]

BTW @takluyver, would be good to bump the __version__ string in terminado post-release to something like 0.6dev or somesuch, master still reports 0.5, so it's a bit confusing by simply checking version strings I had to carefully check that I was really importing from a dev path.

[minrk]

#914 fixes it. It's a combination of changes in terminado and notebook that triggered this - terminado adopting tornado 4 conventions, and a change in the notebook assuming the origin arg is always passed. We usually allow check_origin to be called with no arg, so I restored that in #914.

[ellisonbg]

@minrk thanks for the fix. Other questions:

• The comments in the code suggest we shouldn't need our own custom
  check_origin after we ditch tornado<4 (which we have). Can we just get rid
  of all that in both jupyter/notebook and terminado?
• Should we cut a release of terminado with 4.1?

On Thu, Dec 31, 2015 at 1:45 AM, Min RK [email protected] wrote:

#914 #914 fixes it. It's a
combination of changes in terminado and notebook that triggered this -
terminado adopting tornado 4 conventions, and a change in the notebook
assuming the origin arg is always passed. We usually allow check_origin to
be called with no arg, so I restored that in #914
#914.

—
Reply to this email directly or view it on GitHub
#912 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[minrk]

Terminado technically hasn't dropped support for tornado 3, which is one source of the issue. I opened jupyter/terminado#25 to fix the issue, and jupyter/terminado#26 if we want to drop support for tornado < 4 over there.

We don't need to push a release of terminado with 4.1, but we can do one soon after. There are no issues with released terminado, and it won't be an issue to ensure there aren't with the next release, either.

[ellisonbg]

OK, thanks, I will look at those PRs as well.

But there is a bug that this commit fixed:

jupyter/terminado@eb17fac

since the last release - without that fix, it doesn't work on my latest
ubuntu systems...

On Thu, Dec 31, 2015 at 8:40 AM, Min RK [email protected] wrote:

Terminado technically hasn't dropped support for tornado 3, which is one
source of the issue. I opened jupyter/terminado#25
jupyter/terminado#25 to fix the issue, and
jupyter/terminado#26 jupyter/terminado#26
if we want to drop support for tornado < 4.

We don't need to push a release of terminado with 4.1, but we can do one
soon after. There are no issues with released terminado, and it won't be an
issue to ensure there aren't with the next release, either.

—
Reply to this email directly or view it on GitHub
#912 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[minrk]

If there's an important bug that's fixed we can make a release of terminado. But I don't see a need for coordinated release, because that's not a bug that only affects notebook master and not 4.0, right?

[ellisonbg]

Yep

Sent from my iPhone

On Dec 31, 2015, at 8:47 AM, Min RK [email protected] wrote:

If there's an important bug that's fixed we can make a release of terminado. But I don't see a need for coordinated release, because that's not a bug that only affects notebook master and not 4.0, right?

—
Reply to this email directly or view it on GitHub.

[yuvipanda]

Another issue that'll probably benefit from a terminado bump (or not?) is #104 (which I'm still able to repro on Firefox on Linux with 4.1rc)

[ellisonbg]

Oh, yes - this is a huge improvement - just been using it in the last day.

On Thu, Dec 31, 2015 at 2:19 PM, Yuvi Panda [email protected]
wrote:

Another issue that'll probably benefit from a terminado bump (or not?) is
#104 #104 (which I'm still
able to repro on Firefox on Linux with 4.1rc)

—
Reply to this email directly or view it on GitHub
#912 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[takluyver]

@yuvipanda that probably won't be affected, unfortunately - that's an issue related to term.js, the JS terminal emulator frontend which the notebook bundles. Terminado is the backend infrastructure term.js talks to, and I don't think that needs any changes to support copy & paste.

[takluyver]

Released a new version of terminado which fixes this by depending on Tornado >= 4, and fixes @ellisonbg's issue with 'bad fd' errors.

[ellisonbg]

Thanks!