celluloid / celluloid-io

UNMAINTAINED: See celluloid/celluloid#779 - Evented sockets for Celluloid actors
https://celluloid.io
MIT License
879 stars 93 forks source link

Mailbox receive timeout is not recalculated after IO events #56

Closed halorgium closed 9 years ago

halorgium commented 11 years ago

From #52:

I reverted this patch as it changes the API of Mailbox#receive. This causes more problems than it fixes.

I attempted to move the reactor into the actor. https://github.com/halorgium/celluloid/compare/reactor-in-actor https://github.com/halorgium/celluloid-io/compare/reactor-in-actor

I also have now attempted to wrap the reactor around the mailbox.

https://github.com/halorgium/celluloid/compare/reactor-around-mailbox https://github.com/halorgium/celluloid-io/compare/reactor-around-mailbox

halorgium commented 11 years ago

Currently the API for Mailbox#receive is that it will return nil if no messages match the conditions. The change made in #52 made this also occur if the IO::Reactor handled a IO event.

Unfortunately this meant the Actor#linking_request which does a receive would return nil and presume a timeout.

We need to find a way to correctly support doing a Mailbox#receive outside of the main actor loop.

tarcieri commented 11 years ago

@halorgium some API options for this are:

1) Raise an exception: If we're worried about (JRuby) performance, we can omit the backtrace, as in theory this exception should always be handled internally by Celluloid. Timeouts are arguably an exceptional event so I don't think it's "bad" to use an exception for this purpose. That's how timeout.rb works already (not that we should be copying anything from timeout.rb, just that it isn't unprecedented) 2) Return a "magic symbol": We could return a symbol like :timeout. The downside is this makes :timeout an invalid actor message, which feels a little dirty to me. Of course, there's probably no reason to use a naked :timeout as an actor message anyway.

halorgium commented 10 years ago

3) Wrap the result in an object: this is probably slow for the common case, but would give duck-type-ability.

tarcieri commented 10 years ago

@halorgium wrapping the result in an object works too

godfat commented 10 years ago

If sacrificing performance with objects is fine, then definitely go with objects :)

tarcieri commented 10 years ago

@godfat I guess the question is whether or not timeouts are exceptional enough to warrant being exceptions

godfat commented 10 years ago

I don't yet have a big picture for how celluloid works internally, so I can't really speak if exceptions would make sense or not.

On the other hand, I think due to the way we use Timeout.timeout, it makes sense to raise an exception since that might be the only way to interrupt what is running in the calling thread while we could handle the timeout event in the same thread.

If this timeout is only handled internally in celluloid, and we don't need exceptions for some specific implementation reasons, I would think that we should avoid (asynchronous) exceptions. It's easier to be explicit about what would be returned. (we don't have exception spec in Ruby)

halorgium commented 10 years ago

I agree that this is not an exceptional case. I had a brief talk with @brixen and he suggested the same.

halorgium commented 10 years ago

Another issue which is unrelated to IO is SystemEvents.
It is possible that you will also get a SystemEvent message even if it is not something you were expecting.

tarcieri commented 10 years ago

A potential fix for this problem:

https://github.com/celluloid/celluloid/pull/368

ioquatix commented 9 years ago

I believe this issue has been fixed and can be closed.