circus with new tornado>5.0.2

unkcpz commented 4 years ago

The minimal changes to make circus running with asyncio event loop (tornado>5.0.2).

As mentioned in https://github.com/circus-tent/circus/issues/1124 , but instead of directly upgrade the package to compatible with asyncio I start here try to replace old tornado with tornado>5.0.2 which uses the asyncio event loop. If there is a need to move forward, this is also a good archiving point. The following changes are included:

replace third party mock package to unittest.mock since the py<3 support is deprecated.
add extra [test] packages in setup.py to let running test easier locally
gen.Task is deprecated in new tornado replace it with setting future result in callback
_asyncio.Future can not be inherited and then initialized so MagicMockFuture inherit from concurrent.futures.Future instead.
PeriodicCallback no longer receive loop, have to set event loop to the correct one before running the callback.

In this PR, I only change the code which make all unit tests passed. As addition, also running examples as some sort of blackbox testing. Not sure about are there more code involved with asynchronous programming?

Pinning @biozz @k4nar to have a look at this PR take a moment to review. And tagging @sphuber @ltalirz for suggestions.

tornado>5; <5.0.2 is not supported since there is a bug which prevent event loop from successfully closing fixed in 5.0.2 [1] .

[1] https://www.tornadoweb.org/en/stable/releases/v5.0.2.html

coveralls commented 4 years ago

Coverage decreased (-0.04%) to 63.942% when pulling 518714a201875af9c59c96065f7b6891dd7404a6 on unkcpz:gsoc/new-tornado into ec8e079aa9cc479a91c977e7a0b139cd340573ea on circus-tent:master.

biozz commented 4 years ago

Hi @unkcpz!

I checked your PR and it looks good. I am not that great with asyncio stuff, but I tried starting it and testing it locally, and it works as expected. I would also like to test this PR at my work on a real project. Although we use circus as a plain process manager, it can still give us more confidence for the next circus release.

I have only one small request: could you please add a summary of you changes to the changelog's unreleased section?

unkcpz commented 4 years ago

I would also like to test this PR at my work on a real project. Although we use circus as a plain process manager, it can still give us more confidence for the next circus release.

That will be great! thanks in advance @biozz Code base in our project do not involve too much feature of circus, so that might not cover most of the changes. Your tests will certainly make new release more robust.

I have only one small request: could you please add a summary of you changes to the changelog's unreleased section?

sure, added.

I also propose that there be no rush to completely replace tornado with asyncio before we have more confidence with the new release which will make debug easier.

biozz commented 4 years ago

We've been running this branch of circus for a couple of days now and there are no outstanding issues.

I also noticed, that cpu usage has dropped a bit (cyan line on the screenshot marks the introduction of this branch). I will not deny that this might be a speculation on my part, but I can't share more details about the application due to NDA.

unkcpz commented 4 years ago

Thanks @biozz and it is a good news that the cpu usage is drooped. looking forward this pr to be merged anytime you feel suitable.

ltalirz commented 4 years ago

@k4nar would you mind creating a v0.17.0 release for this?

ltalirz commented 4 years ago

@k4nar Is there a reason for holding off the v0.17.0 release? Would you like help with preparing the release?

Edit: Ah, I see that we are waiting for https://github.com/circus-tent/circus/pull/1132 to be merged

circus-tent / circus

circus with new tornado>5.0.2 #1129