WIP: more fixes for tornado_pyuv.py

benoitc / gaffer

control, watch and launch your applications and jobs over HTTP.

Other

367 stars 34 forks source link

WIP: more fixes for tornado_pyuv.py #27

Closed saghul closed 11 years ago

saghul commented 11 years ago

Removed unused run_loop argument from start
Use the more efficient Loop.run on start
Close file descriptors on close
Removed unused all_handlers argument

Don't merge yet.

saghul commented 11 years ago

Hum, some tests seem to be failing, I'll run the test suite locally...

saghul commented 11 years ago

While attempting to fix the failing tests in test_lookup I think I found a bigger problem underneath:

LookupClient gets a reference to the pyuv loop.
The, LookupClient creates an IOLoop instance with the given loop (maybe we should pass the IOLoop instance instead?)
The t0 timer on test_lookup_service kicks in after 0.4 seconds and closes the LookupClient
LookupClient closes the IOLoop on its _terminate function, thus all pyuv handles are closed

I lost track of the internals after the big refactor :-) but the changes I'm proposing here are those available in my tornado-pyuv repo, which pass the entire Tornado test suite, so I think there may be some internal problems in gaffer like the one I described above.

More over, I see pyuv handles and the IOLoop used together, which is a bit confusing. Could we just use the Tornado IOLoop for everything? Using the pyuv implementation, that is.

benoitc commented 11 years ago

I don't really want to use tornado as the first citizen in gaffer. Imo the ioloop should be just used as a wrapper around pyuv.Loop and then close only handlerslaunched using this ioloop.

I guess in that case the client could receive the ioloop, but i'm worried about what it can imply if later (as I intend to) the client is only based on pyuv (and http-parser). Maybe this isn't a problem?

saghul commented 11 years ago

Ok, got it. In that case I'll modify IOLoop.close not to close all handles in the loop. We can also get rid of the all_fds paramete, because, frankly, when don't you want to close the file descriptors? So IOLoop.close will just close the poll handles it started and the associated file descriptors. This should hopefully do.

benoitc commented 11 years ago

Sounds good indeed. Thanks for that :)

saghul commented 11 years ago

Tests pass on OSX, come on Travis, give me some good news!

saghul commented 11 years ago

Tests pass on Travis for 2.7, but building the environment seems to fail for 3.3. Though it's not related to this changes.

saghul commented 11 years ago

Heh, it took longer than expected, but tests also pass on 3.3 :-)

benoitc commented 11 years ago

Thanks! Patch looks good for me. I will mege it in some mn.

benoitc commented 11 years ago

applied in last head, thanks!