On Mon, Oct 29, 2012 at 5:43 PM, Yury Selivanov firstname.lastname@example.org wrote:
Finally got some time to do a review & read what others posted.
Some comments are more general, some are more implementation-specific (hopefully you want to hear latter ones as well)
And I'm still in the process of digesting your approach & code (as I've spent too much time with my implementation)...
On 2012-10-28, at 7:52 PM, Guido van Rossum email@example.com wrote: [...]
- I'd make EventLoopMixin a separate entity from pollsters. So that you'd
be able to add many different pollsters to one EventLoop. This way you can have specialized pollster for different types of IO, including UI etc.
I came to the same conclusion, so I fixed this. See the latest version.
(BTW, I also renamed add_reader() etc. on the Pollster class to register_reader() etc. -- I dislike similar APIs on different classes to have the same name if there's not a strict super class override involved.)
- Sometimes, there is a need to run a coroutine in a threadpool. I know it
sounds weird, but it's probably worth exploring.
I think that can be done quite simply. Since each thread has its own eventloop (via the magic of TLS), it's as simple as writing a function that creates a task, starts it, and then runs the eventloop. There's nothing else running in that particular thread, and its eventloop will terminate when there's nothing left to do there -- i.e. when the task is done. Sketch:
def some_generator(arg): ...stuff using yield from... return 42
def run_it_in_the_threadpool(arg): t = Task(some_generator(arg)) t.start() scheduling.run() return t.result
# And in your code: result = yield from scheduling.call_in_thread(run_it_in_the_threadpool, arg)
# Now result == 42.
- In my framework each threadpool worker has its own local context, with
various information like what Task run the operation etc.
I think I have this too -- Thread-Local Storage!
And few small things:
- epoll.poll and other syscalls need to be wrapped in try..except to catch
and ignore (and log?) EINTR type of exceptions.
- For epoll you probably want to check/(log?) EPOLLHUP and EPOLLERR errors
Do you have a code sample? I haven't found a need yet.
In the docstrings I use the prefix "COROUTINE:" to indicate public APIs that should be invoked using yield from.
As others, I would definitely suggest adding a decorator to make coroutines more distinguishable.
That's definitely on my TODO list.
It would be even better if we can return a tiny wrapper, that lets you to simply write 'doit.run().with_timeout(2.1)', instead of:
task = scheduling.Task(doit(), timeout=2.1) task.start() scheduling.run()
The run() call shouldn't be necessary unless you are at the toplevel.
And avoid manual Task instantiation at all.
Hm. I want the generator function to return just a generator object, and I can't add methods to that. But we can come up with a decent API.
I also liked the simplicity of the Task class. I think it'd be easy to mix greenlets in it by switching in a new greenlet on each 'step'. That will give you 'yield_()' function, which you can use in the same way you use 'yield' statement now (I'm not proposing to incorporate greenlets in the lib itself, but rather to provide an option to do so) Hence there should be a way to plug your own Task (sub-)class in.
Hm. Someone else will have to give that a try.
Thanks for your feedback!!