search

celluloid / celluloid-redis

UNMAINTAINED: See celluloid/celluloid#779
MIT License
50 stars 9 forks source link

Implement connection timeout #6

Closed simonrobson closed 11 years ago

simonrobson commented 11 years ago

Is there a downside to implementing the connection timeout using the Future timeout feature?

I originally started out with your suggestion to implement a first-class Celluloid::Timeout. But my first cut at that used this same Future-based strategy: gist

coveralls commented 11 years ago

Coverage increased (+0.21%) when pulling 9bf7c17aff031c5d8832a859cb74d03291e00d4b on simonrobson:connection-timeout into 08f3f1cb94ae44a82449a47784df149488e6b93f on celluloid:master.

View Details

tarcieri commented 11 years ago

The main problem with this implementation is it will incur a thread per operation, which is probably ok, but would be slower than a proper timeout system which can use the reactor to implement timeouts. The other problem is futures aren't really cancelable at present.

A better approach would be to configure a Celluloid timer for the timeout (e.g. after(options[:timeout]) { ... }) although Celluloid doesn't presently expose an API to cancel an active task (although it does have an eternal API where you resume a task with DeadTaskError)

This is why I really want to get all this logic wrapped up in an extremely simple API that does everything I just described above: sets a timer, and resumes the task with a DeadTaskError (or perhaps Celluloid::TimeoutError which subclasses DeadTaskError) if it times out and needs to be cancelled.

simonrobson commented 11 years ago

OK, that makes sense. Will close this PR for now, and see if I can come up with something closer to what you describe. Thanks.