Fix PeriodicCallback usage for Tornado 5

[pitrou]

The io_loop argument to PeriodicCallback is removed in Tornado 5.0.

[mrocklin]

Do we have a valid usecase here? Should we try to encourage upstream not to make this change?

[pitrou]

I explained our use case there: https://groups.google.com/d/msg/python-tornado/7JNWKwCTvZs/vI8gdPppAAAJ . I'm not sure @bdarnell is convinced by the argument :-)

[mrocklin]

I'm inclined to wait on the Tornado 5.0 design to be finalized before merging this

[pitrou]

That sounds reasonable.

[bdarnell]

I'd definitely recommend waiting until the Tornado 5.0 design is finalized before merging this, but it's good to have this branch prepared because I'm going to ask you to test a few more things along the way. :)

I'd be happy to meet you halfway on this change: If I move the initialization of self.io_loop from PeriodicCallback.__init__ to PeriodicCallback.start, then you won't need to carry your own version of this class (if I'm understanding everything correctly). You'd still need to remove the io_loop arguments, though, and I'm not sure if it would be possible to do that and retain compatibility with both 4.5 and 5.0 (without more invasive changes to create the PeriodicCallback objects on the thread where they will be used).

[pitrou]

You'd still need to remove the io_loop arguments, though, and I'm not sure if it would be possible to do that and retain compatibility with both 4.5 and 5.0

We could have a thin wrapper which switches based on the Tornado version. Something like:

def PeriodicCallback(callback, callback_time, io_loop=None):
    if tornado.version_info >= (5,):
        return tornado.ioloop.PeriodicCallback(callback, callback_time)
    else:
        return tornado.ioloop.PeriodicCallback(callback, callback_time, io_loop)

[pitrou]

If I move the initialization of self.io_loop from PeriodicCallback.init to PeriodicCallback.start, then you won't need to carry your own version of this class (if I'm understanding everything correctly).

@bdarnell, would you be ready to take a PR doing just this?
(edit: silly typo)

[bdarnell]

If I move the initialization of self.io_loop from PeriodicCallback.init to PeriodicCallback.start, then you won't need to carry your own version of this class (if I'm understanding everything correctly).

@bdarnell, would you be ready to take a PR doing just this?
(edit: silly typo)

Yes.

[mrocklin]

It's nice to see the PeriodicCallback class go away

[mrocklin]

Do we ever actually provide an io_loop keyword? If not then can we remove this function entirely?

[pitrou]

Well, yes, we do, in a lot of places :-)

[mrocklin]

It seems odd to me that we are allowed to ignore those imports in Tornado >= 5.0 but need them in <= 5.0.

[pitrou]

That's because we fixed PeriodicCallback so that the IO loop is looked up when its start method is called. Before 5.0 the IO loop is set in the constructor, and we call it from another thread.

[pitrou]

@mrocklin are you ok with merging this?

[mrocklin]

Yes. This looks good to me.

I'm looking forward to getting closer to being able to actually try out the new TCPStream stuff on real hardware and this will help.