New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing behavior when multiple events for a single socket are fired in a single event loop iteration #184

Closed
carllerche opened this Issue May 28, 2015 · 49 comments

Comments

Projects
None yet
7 participants
@carllerche
Owner

carllerche commented May 28, 2015

Issue representing the confusion experienced in #172 and #179.

My current understanding of the confusion is, given epoll & a socket that received a HUP and is writable, mio will first call the Handler::readable w/ ReadHint set to Hup followed by the Handler::writable event. The confusion arises when the socket is closed during the Handler::readable call, or perhaps the token associated with that token is reused. The next call to Handler::writable for the "finalized" socket is unexpected.

Part of the original API design consideration is how to unify epoll w/ kqueue. With kqueue, the readable & writable notifications are provided separately. As far as I am aware, there is no guarantee as to which order (readable or writable) the events will be seen.

I don't believe that either solutions described by @rrichardson in #179 would be possible to implement (efficiently) for kqueue. The reason why hup / error are provided as a ReadHint is that it is not 100% portable for mio to detect them. However, the hup / error state can be discovered by the mio by reading from the socket, which is why they are considered readable events. In other words, to correctly use mio, hup / error states must be discovered by listening for Handler::readable, reading from the socket, and seeing an error. As such, doing anything with the ReadHint argument passed to Handler::readable is simply an optimization. I also don't see a way to implement passing ReadHint (renamed to EventHint) to Handler::writable callback in a portable fashion.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson May 28, 2015

Contributor

In this case, the confusion arose when the user got a hup and then a writable event for the same token. In isolation, the obvious thing to do when you get a hup event is to unregister the socket and remove it and the token from the slab. However, immediately, the writable event is called for the token, regardless of what one does with the token. This results in unexpected behavior.

Given the current design, one would have to update the state in their connection's state machine indicating a disconnection, so that they can ignore the writable callback.

An approach that would allow them to not have to manage a potential disconnection in their state machine is to simply pass the ReadHint information to both readable and writable, then the convention would be to check the Hint for err/hup before touching the socket. (more specifically, on a read event, one would likely read then deregister, and on a write event, they would just deregister)

If they're on OSX, then they'll just have to understand that they'll never get formal hup/err hints (afaik) so nothing really changes.

Contributor

rrichardson commented May 28, 2015

In this case, the confusion arose when the user got a hup and then a writable event for the same token. In isolation, the obvious thing to do when you get a hup event is to unregister the socket and remove it and the token from the slab. However, immediately, the writable event is called for the token, regardless of what one does with the token. This results in unexpected behavior.

Given the current design, one would have to update the state in their connection's state machine indicating a disconnection, so that they can ignore the writable callback.

An approach that would allow them to not have to manage a potential disconnection in their state machine is to simply pass the ReadHint information to both readable and writable, then the convention would be to check the Hint for err/hup before touching the socket. (more specifically, on a read event, one would likely read then deregister, and on a write event, they would just deregister)

If they're on OSX, then they'll just have to understand that they'll never get formal hup/err hints (afaik) so nothing really changes.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche May 28, 2015

Owner

The current behavior / API is also problematic when dealing with the remote end shutting down part of the connection....

Owner

carllerche commented May 28, 2015

The current behavior / API is also problematic when dealing with the remote end shutting down part of the connection....

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson May 28, 2015

Contributor

Yes. That scenario applies to my above comment.

ping @dpc and @hoxnox

Contributor

rrichardson commented May 28, 2015

Yes. That scenario applies to my above comment.

ping @dpc and @hoxnox

@hoxnox

This comment has been minimized.

Show comment
Hide comment
@hoxnox

hoxnox May 29, 2015

Contributor

I couldn't live with that API and fell down to nix-rust and epoll. Fortunately my target platform is Linux. Surprisingly code became simpler, cleaner and more stable.
As for me, if I have to use something cross-platform, I would rather port libevent.

Contributor

hoxnox commented May 29, 2015

I couldn't live with that API and fell down to nix-rust and epoll. Fortunately my target platform is Linux. Surprisingly code became simpler, cleaner and more stable.
As for me, if I have to use something cross-platform, I would rather port libevent.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc May 29, 2015

Contributor

I can confirm that this is confusing. I don't have much insight on a best design, but here's my view. From my perspective breaking up events into separate handler is not a best idea. It seems to me if one handler would be called for group of events, with an argument what happened than it might be better. On some backends it might always be single event, on some there might be grouping.

I find current way inconsistent. Eg. Why is hangup passed as an argument to read? If there's already argument passed on what type of event happened, why not go all the way with it.

In my code for every handler called, I need to do a lookup of an object to act on. If I had the events as a bitmask, than I can act on all events grouped together, with just one lookup.

And of course, if the events are not artificially broken up, there is no problem of ghost events after deregister.

Contributor

dpc commented May 29, 2015

I can confirm that this is confusing. I don't have much insight on a best design, but here's my view. From my perspective breaking up events into separate handler is not a best idea. It seems to me if one handler would be called for group of events, with an argument what happened than it might be better. On some backends it might always be single event, on some there might be grouping.

I find current way inconsistent. Eg. Why is hangup passed as an argument to read? If there's already argument passed on what type of event happened, why not go all the way with it.

In my code for every handler called, I need to do a lookup of an object to act on. If I had the events as a bitmask, than I can act on all events grouped together, with just one lookup.

And of course, if the events are not artificially broken up, there is no problem of ghost events after deregister.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche May 30, 2015

Owner

@dpc @hoxnox The problem is that events are broken up on kqueue. This is going to require a bit of thought... I'm all for unifying the Handler callback to a ready, but I am not 100% sure how best to support both epoll & kqueue in a portable way.

Owner

carllerche commented May 30, 2015

@dpc @hoxnox The problem is that events are broken up on kqueue. This is going to require a bit of thought... I'm all for unifying the Handler callback to a ready, but I am not 100% sure how best to support both epoll & kqueue in a portable way.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche May 30, 2015

Owner

@dpc Also, for what it is worth, Poll is a lower level wrapper around both epoll & kqueue that doesn't worry about normalizing events.

Owner

carllerche commented May 30, 2015

@dpc Also, for what it is worth, Poll is a lower level wrapper around both epoll & kqueue that doesn't worry about normalizing events.

@hoxnox

This comment has been minimized.

Show comment
Hide comment
@hoxnox

hoxnox May 30, 2015

Contributor

@carllerche libevent supports kqueue as well as epoll and, as I know, there is no problems with it. Maybe it's sources helps.

Contributor

hoxnox commented May 30, 2015

@carllerche libevent supports kqueue as well as epoll and, as I know, there is no problems with it. Maybe it's sources helps.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc May 31, 2015

Contributor

@carllerche : I don't understand. What I'm saying is that mio should not additionally break up events.

pub trait Handler {
    ...
    fn readable(&mut self, event_loop: &mut EventLoop<Self>, token: Token, hint: ReadHint) { ... }
    fn writable(&mut self, event_loop: &mut EventLoop<Self>, token: Token) { ... }
    ....
}

Could be changed to

pub trait Handler {
    ...
    fn event(&mut self, event_loop: &mut EventLoop<Self>, token: Token, event: EventBitmask) { ... }
    ....
}

EventBitmask would contain bits for "readable, writeable, hup" and maybe others.

Now if the backend is always reporting one event at the time, than only one bit will be set. If backend supports reporting more events in the same event, then more bits will be set. Handler code will handle both cases just fine.

Now the handler will go through events and check them in order it wants.

Contributor

dpc commented May 31, 2015

@carllerche : I don't understand. What I'm saying is that mio should not additionally break up events.

pub trait Handler {
    ...
    fn readable(&mut self, event_loop: &mut EventLoop<Self>, token: Token, hint: ReadHint) { ... }
    fn writable(&mut self, event_loop: &mut EventLoop<Self>, token: Token) { ... }
    ....
}

Could be changed to

pub trait Handler {
    ...
    fn event(&mut self, event_loop: &mut EventLoop<Self>, token: Token, event: EventBitmask) { ... }
    ....
}

EventBitmask would contain bits for "readable, writeable, hup" and maybe others.

Now if the backend is always reporting one event at the time, than only one bit will be set. If backend supports reporting more events in the same event, then more bits will be set. Handler code will handle both cases just fine.

Now the handler will go through events and check them in order it wants.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson May 31, 2015

Contributor

I don't entirely disagree here. There is a number (technically infinite) of
event types that a poller could return, dividing into only read/write is
somewhat arbitrary.

On Sun, May 31, 2015, 5:02 PM Dawid Ciężarkiewicz notifications@github.com
wrote:

@carllerche https://github.com/carllerche : I don't understand. What
I'm saying is that mio should not additionally break up events.

pub trait Handler {
...
fn readable(&mut self, event_loop: &mut EventLoop, token: Token, hint: ReadHint) { ... }
fn writable(&mut self, event_loop: &mut EventLoop, token: Token) { ... }
....
}

Could be changed to

pub trait Handler {
...
fn event(&mut self, event_loop: &mut EventLoop, token: Token, event: EventBitmask) { ... }
....
}

EventBitmask would contain bits for "readable, writeable, hup" and maybe
others.

Now if the backend is always reporting one event at the time, than only
one bit will be set. If backend supports reporting more events in the same
event, then more bits will be set. Handler code will handle both cases just
fine.

Now the handler will go through events and check them in order it wants.


Reply to this email directly or view it on GitHub
#184 (comment).

Contributor

rrichardson commented May 31, 2015

I don't entirely disagree here. There is a number (technically infinite) of
event types that a poller could return, dividing into only read/write is
somewhat arbitrary.

On Sun, May 31, 2015, 5:02 PM Dawid Ciężarkiewicz notifications@github.com
wrote:

@carllerche https://github.com/carllerche : I don't understand. What
I'm saying is that mio should not additionally break up events.

pub trait Handler {
...
fn readable(&mut self, event_loop: &mut EventLoop, token: Token, hint: ReadHint) { ... }
fn writable(&mut self, event_loop: &mut EventLoop, token: Token) { ... }
....
}

Could be changed to

pub trait Handler {
...
fn event(&mut self, event_loop: &mut EventLoop, token: Token, event: EventBitmask) { ... }
....
}

EventBitmask would contain bits for "readable, writeable, hup" and maybe
others.

Now if the backend is always reporting one event at the time, than only
one bit will be set. If backend supports reporting more events in the same
event, then more bits will be set. Handler code will handle both cases just
fine.

Now the handler will go through events and check them in order it wants.


Reply to this email directly or view it on GitHub
#184 (comment).

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson May 31, 2015

Contributor

I do run into this issue in my reactor framework. I have to store the fact
that I got a failed connection from my read handler knowing that the write
handler is about to be spuriously called.

On Sun, May 31, 2015, 5:06 PM Rick Richardson rick.richardson@gmail.com
wrote:

I don't entirely disagree here. There is a number (technically infinite)
of event types that a poller could return, dividing into only read/write is
somewhat arbitrary.

On Sun, May 31, 2015, 5:02 PM Dawid Ciężarkiewicz <
notifications@github.com> wrote:

@carllerche https://github.com/carllerche : I don't understand. What
I'm saying is that mio should not additionally break up events.

pub trait Handler {
...
fn readable(&mut self, event_loop: &mut EventLoop, token: Token, hint: ReadHint) { ... }
fn writable(&mut self, event_loop: &mut EventLoop, token: Token) { ... }
....
}

Could be changed to

pub trait Handler {
...
fn event(&mut self, event_loop: &mut EventLoop, token: Token, event: EventBitmask) { ... }
....
}

EventBitmask would contain bits for "readable, writeable, hup" and maybe
others.

Now if the backend is always reporting one event at the time, than only
one bit will be set. If backend supports reporting more events in the same
event, then more bits will be set. Handler code will handle both cases just
fine.

Now the handler will go through events and check them in order it wants.


Reply to this email directly or view it on GitHub
#184 (comment).

Contributor

rrichardson commented May 31, 2015

I do run into this issue in my reactor framework. I have to store the fact
that I got a failed connection from my read handler knowing that the write
handler is about to be spuriously called.

On Sun, May 31, 2015, 5:06 PM Rick Richardson rick.richardson@gmail.com
wrote:

I don't entirely disagree here. There is a number (technically infinite)
of event types that a poller could return, dividing into only read/write is
somewhat arbitrary.

On Sun, May 31, 2015, 5:02 PM Dawid Ciężarkiewicz <
notifications@github.com> wrote:

@carllerche https://github.com/carllerche : I don't understand. What
I'm saying is that mio should not additionally break up events.

pub trait Handler {
...
fn readable(&mut self, event_loop: &mut EventLoop, token: Token, hint: ReadHint) { ... }
fn writable(&mut self, event_loop: &mut EventLoop, token: Token) { ... }
....
}

Could be changed to

pub trait Handler {
...
fn event(&mut self, event_loop: &mut EventLoop, token: Token, event: EventBitmask) { ... }
....
}

EventBitmask would contain bits for "readable, writeable, hup" and maybe
others.

Now if the backend is always reporting one event at the time, than only
one bit will be set. If backend supports reporting more events in the same
event, then more bits will be set. Handler code will handle both cases just
fine.

Now the handler will go through events and check them in order it wants.


Reply to this email directly or view it on GitHub
#184 (comment).

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche May 31, 2015

Owner

@dpc I'm not against this, but could you elaborate on how this would behave with kqueue?

Owner

carllerche commented May 31, 2015

@dpc I'm not against this, but could you elaborate on how this would behave with kqueue?

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Jun 1, 2015

Contributor

@carllerche : Unfortunately my kqueue knowledge is null. What is the problem there?

The problem is that events are broken up on kqueue.

The way I understand it, kqueue signals single event every time, and EventBitmask from my example will always have only one bit set?

Contributor

dpc commented Jun 1, 2015

@carllerche : Unfortunately my kqueue knowledge is null. What is the problem there?

The problem is that events are broken up on kqueue.

The way I understand it, kqueue signals single event every time, and EventBitmask from my example will always have only one bit set?

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jun 1, 2015

Owner

@dpc Kqueue treats readable & writable events on a socket as completely separate (unlike epoll). See here.

Basically, the current mio design of having separate events for readable and writable was made to handle kqueue while adding as little overhead as possible. The strategy to implement an event function that coalesces both readable & writable events that works on kqueue does not seem obvious to me at first thought.

Owner

carllerche commented Jun 1, 2015

@dpc Kqueue treats readable & writable events on a socket as completely separate (unlike epoll). See here.

Basically, the current mio design of having separate events for readable and writable was made to handle kqueue while adding as little overhead as possible. The strategy to implement an event function that coalesces both readable & writable events that works on kqueue does not seem obvious to me at first thought.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 1, 2015

Contributor

I see two options:

  1. Coalesce the event masks into a single mask per file-descriptor and then
    invoke the event() callback.
  2. Just call the event() callback multiple times, one for each event.

Since kqueue and others can produce user defined events, I am wondering if
there is a way to extend the amount/type of events event_loop can produce.

On Mon, Jun 1, 2015 at 1:39 PM Carl Lerche notifications@github.com wrote:

@dpc https://github.com/dpc Kqueue treats readable & writable events on
a socket as completely separate (unlike epoll). See here
https://github.com/carllerche/mio/blob/master/src/sys/unix/kqueue.rs#L39-L40
.

Basically, the current mio design of having separate events for readable
and writable was made to handle kqueue while adding as little overhead as
possible. The strategy to implement an event function that coalesces both
readable & writable events that works on kqueue does not seem obvious to me
at first thought.


Reply to this email directly or view it on GitHub
#184 (comment).

Contributor

rrichardson commented Jun 1, 2015

I see two options:

  1. Coalesce the event masks into a single mask per file-descriptor and then
    invoke the event() callback.
  2. Just call the event() callback multiple times, one for each event.

Since kqueue and others can produce user defined events, I am wondering if
there is a way to extend the amount/type of events event_loop can produce.

On Mon, Jun 1, 2015 at 1:39 PM Carl Lerche notifications@github.com wrote:

@dpc https://github.com/dpc Kqueue treats readable & writable events on
a socket as completely separate (unlike epoll). See here
https://github.com/carllerche/mio/blob/master/src/sys/unix/kqueue.rs#L39-L40
.

Basically, the current mio design of having separate events for readable
and writable was made to handle kqueue while adding as little overhead as
possible. The strategy to implement an event function that coalesces both
readable & writable events that works on kqueue does not seem obvious to me
at first thought.


Reply to this email directly or view it on GitHub
#184 (comment).

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jun 1, 2015

Owner

@rrichardson how would you implement option 1? The best that I can think of is to either use a map lookup structure to coalesce the events each iteration or use a Slab to map every token registered w/ EventLoop with the event masks.

Owner

carllerche commented Jun 1, 2015

@rrichardson how would you implement option 1? The best that I can think of is to either use a map lookup structure to coalesce the events each iteration or use a Slab to map every token registered w/ EventLoop with the event masks.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 1, 2015

Contributor

I can't think of a way to do it other than some sort of associative map.
It would probably be optimal to use a local radix tree that is sized
directly to the amount of fds that kevent() can return at one time.

create map;
iterate through kevent array, inserting and updating map;
iterate through map invoking callbacks;

On Mon, Jun 1, 2015 at 2:49 PM Carl Lerche notifications@github.com wrote:

@rrichardson https://github.com/rrichardson how would you implement
option 1? The best that I can think of is to either use a hash lookup
structure to coalesce the events each iteration or use a Slab to map
every token registered w/ EventLoop with the event masks.


Reply to this email directly or view it on GitHub
#184 (comment).

Contributor

rrichardson commented Jun 1, 2015

I can't think of a way to do it other than some sort of associative map.
It would probably be optimal to use a local radix tree that is sized
directly to the amount of fds that kevent() can return at one time.

create map;
iterate through kevent array, inserting and updating map;
iterate through map invoking callbacks;

On Mon, Jun 1, 2015 at 2:49 PM Carl Lerche notifications@github.com wrote:

@rrichardson https://github.com/rrichardson how would you implement
option 1? The best that I can think of is to either use a hash lookup
structure to coalesce the events each iteration or use a Slab to map
every token registered w/ EventLoop with the event masks.


Reply to this email directly or view it on GitHub
#184 (comment).

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Jun 1, 2015

Contributor

Sorry, I still don't understand where is the problem, @carllerche . In my example on both readable and writeable you call the same event with argument bitmask having different bit set. There is no need to coalesce anything artificially, if the backend did not report it already together. What am I missing?

Contributor

dpc commented Jun 1, 2015

Sorry, I still don't understand where is the problem, @carllerche . In my example on both readable and writeable you call the same event with argument bitmask having different bit set. There is no need to coalesce anything artificially, if the backend did not report it already together. What am I missing?

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jun 1, 2015

Owner

@dpc Doing this would mean that the original confusing behavior would still be around, but only on some platforms making code that is written w/ mio still have to factor it in but also have to handle differences between platforms.

Currently, if you want to drop down to a lower level and handle platform differences yourself, you can use the Poll type which will just provide you with the events directly.

Owner

carllerche commented Jun 1, 2015

@dpc Doing this would mean that the original confusing behavior would still be around, but only on some platforms making code that is written w/ mio still have to factor it in but also have to handle differences between platforms.

Currently, if you want to drop down to a lower level and handle platform differences yourself, you can use the Poll type which will just provide you with the events directly.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Jun 1, 2015

Contributor

Why, because underlying backend can report additional events on it's own?

Contributor

dpc commented Jun 1, 2015

Why, because underlying backend can report additional events on it's own?

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 1, 2015

Contributor

kqueue will produce N event structures for a single file descriptor in a single call of kevent. Those events are intermixed with all of the events for all of the other fds it is monitoring. So if you want a single callback per descriptor that is effected, then we absolutely need to coalesce the events into a single bitmask.

epoll is easier, to be sure, it coalesces the events for us, so we only get one event structure per fd after polling.

My suggestion is that we fix the "bug" in the interrim by not invoking readable for HUP/ERR, but rather pass ReadHint to both readable and writable events when found. This should work for both epoll and kqueue.

My understanding of kqueue and EV_EOF is sketchy, but it sounds like kqueue users aren't used to getting hup or error notifications anyways, they just get those from the socket/fd.

Contributor

rrichardson commented Jun 1, 2015

kqueue will produce N event structures for a single file descriptor in a single call of kevent. Those events are intermixed with all of the events for all of the other fds it is monitoring. So if you want a single callback per descriptor that is effected, then we absolutely need to coalesce the events into a single bitmask.

epoll is easier, to be sure, it coalesces the events for us, so we only get one event structure per fd after polling.

My suggestion is that we fix the "bug" in the interrim by not invoking readable for HUP/ERR, but rather pass ReadHint to both readable and writable events when found. This should work for both epoll and kqueue.

My understanding of kqueue and EV_EOF is sketchy, but it sounds like kqueue users aren't used to getting hup or error notifications anyways, they just get those from the socket/fd.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jun 2, 2015

Owner

@rrichardson Would a radix tree be better than some sort of simple hashing? Right now, kqueue is configured to pull in at most 1024 events. Given that, it seems that a 4k vec could be allocated and some very simple hashing strategy used... (very cheap hash function & robin hood hash collision strategy)

The hashing algorithm could be as simple as masking the lower 12 bits of the token. Since tokens are usually taken from Slab, and Slab uses numbers starting at 0 and increments (and reuses slots that are cleared), in most simple cases there should be no collisions... in most cases, it would take more than 4k live connections to start hitting collisions, in which case there would be some linear probing.

The size of the original vec could also be configured, so if using kqueue w/ very large numbers of live connections, the vec size could be increased to reduce collisions.

Owner

carllerche commented Jun 2, 2015

@rrichardson Would a radix tree be better than some sort of simple hashing? Right now, kqueue is configured to pull in at most 1024 events. Given that, it seems that a 4k vec could be allocated and some very simple hashing strategy used... (very cheap hash function & robin hood hash collision strategy)

The hashing algorithm could be as simple as masking the lower 12 bits of the token. Since tokens are usually taken from Slab, and Slab uses numbers starting at 0 and increments (and reuses slots that are cleared), in most simple cases there should be no collisions... in most cases, it would take more than 4k live connections to start hitting collisions, in which case there would be some linear probing.

The size of the original vec could also be configured, so if using kqueue w/ very large numbers of live connections, the vec size could be increased to reduce collisions.

@reem

This comment has been minimized.

Show comment
Hide comment
@reem

reem Jun 3, 2015

Contributor

Recording some thoughts from IRC:

An alternative approach to solving this is to instead make deregistering an asynchronous operation, so the user is on the hook for handling events on deregistered tokens/fds until that token is confirmed deregistered.

There are several ways to accomplish this. The ideal interface is to add another method on Handler, lets say deregistered, which gets called with a Token once it is deregistered and no more events are pending on it. However, this requires some slight overhead within mio to track deregistrations, which is not ideal.

A lower-implied-overhead approach is to instead add another method to Handler, lets say tick, which is called every time the event loop has entirely finished dispatching events from the current iteration before the next call into epoll or kqueue. Deregistrations "take effect" on every call to tick, which would then be the place for users to act on those deregistrations. This approach practically require users to maintain a list of deregistrations between tick calls, but it is much less opinionated than doing the same within mio.

Contributor

reem commented Jun 3, 2015

Recording some thoughts from IRC:

An alternative approach to solving this is to instead make deregistering an asynchronous operation, so the user is on the hook for handling events on deregistered tokens/fds until that token is confirmed deregistered.

There are several ways to accomplish this. The ideal interface is to add another method on Handler, lets say deregistered, which gets called with a Token once it is deregistered and no more events are pending on it. However, this requires some slight overhead within mio to track deregistrations, which is not ideal.

A lower-implied-overhead approach is to instead add another method to Handler, lets say tick, which is called every time the event loop has entirely finished dispatching events from the current iteration before the next call into epoll or kqueue. Deregistrations "take effect" on every call to tick, which would then be the place for users to act on those deregistrations. This approach practically require users to maintain a list of deregistrations between tick calls, but it is much less opinionated than doing the same within mio.

@reem

This comment has been minimized.

Show comment
Hide comment
@reem

reem Jun 3, 2015

Contributor

Note that adding tick or deregistered is orthogonal to collapsing readable and writable into one method, which could be done or not either way.

Contributor

reem commented Jun 3, 2015

Note that adding tick or deregistered is orthogonal to collapsing readable and writable into one method, which could be done or not either way.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Jun 3, 2015

Contributor

The tick imposes on the user the same tracking that mio would have to do internally for deregistered case, doesn't it? In that case it would be nicer if mio did it once and well, instead of letting user do it slowly or incorrectly.

Contributor

dpc commented Jun 3, 2015

The tick imposes on the user the same tracking that mio would have to do internally for deregistered case, doesn't it? In that case it would be nicer if mio did it once and well, instead of letting user do it slowly or incorrectly.

@reem

This comment has been minimized.

Show comment
Hide comment
@reem

reem Jun 3, 2015

Contributor

Yea, but it would allow different implementations depending on the concerns of the user. This is similar to the overhead "imposed" by mio due to the token system.

Contributor

reem commented Jun 3, 2015

Yea, but it would allow different implementations depending on the concerns of the user. This is similar to the overhead "imposed" by mio due to the token system.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 3, 2015

Contributor

Having a final "no more events for this iteration" achieves the same effect as ensuring that all events are sent in a single event(), both have their pros and cons. The final handler would be easier to implement in the general case, the single-handler approach is certainly simpler from an a trait interface perspective. Also, I do want to point out that in either of these cases, an async deregister aren't really necessary. It is safe in Mio to deregister in an event callback.

The async deregister came up because we were discussing the specific case in libuv and higher level mio frameworks where the callback handler is registered with the token, so it takes some special gymnastics to manage.

Contributor

rrichardson commented Jun 3, 2015

Having a final "no more events for this iteration" achieves the same effect as ensuring that all events are sent in a single event(), both have their pros and cons. The final handler would be easier to implement in the general case, the single-handler approach is certainly simpler from an a trait interface perspective. Also, I do want to point out that in either of these cases, an async deregister aren't really necessary. It is safe in Mio to deregister in an event callback.

The async deregister came up because we were discussing the specific case in libuv and higher level mio frameworks where the callback handler is registered with the token, so it takes some special gymnastics to manage.

@wrl

This comment has been minimized.

Show comment
Hide comment
@wrl

wrl Jun 3, 2015

While it may be safe in Mio to de-register a token in the midst of handling events, client code still needs to know when it's safe to release data structures associated with the token. Otherwise, the token exists in a sort of gray area, not dissimilar to a libuv handle being stuck with UV_CLOSING set, without a state transition to UV_CLOSED.

wrl commented Jun 3, 2015

While it may be safe in Mio to de-register a token in the midst of handling events, client code still needs to know when it's safe to release data structures associated with the token. Otherwise, the token exists in a sort of gray area, not dissimilar to a libuv handle being stuck with UV_CLOSING set, without a state transition to UV_CLOSED.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 3, 2015

Contributor

Definitely, but if event() is only fired a max of one time per iteration, then we know that callback is final. The owner of the data structures doesn't need a state machine in that case.

Frankly, if we can get some nods in either direction I'm happy to put in a PR for either approach. Or we can ask the benevolent dictator :P

Contributor

rrichardson commented Jun 3, 2015

Definitely, but if event() is only fired a max of one time per iteration, then we know that callback is final. The owner of the data structures doesn't need a state machine in that case.

Frankly, if we can get some nods in either direction I'm happy to put in a PR for either approach. Or we can ask the benevolent dictator :P

@wrl

This comment has been minimized.

Show comment
Hide comment
@wrl

wrl Jun 3, 2015

What about the case of, say, token1 de-registering token2 before token2 has had its events dispatched?

wrl commented Jun 3, 2015

What about the case of, say, token1 de-registering token2 before token2 has had its events dispatched?

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 3, 2015

Contributor

When a poll() waits and completes is effectively arbitrary, but it probably does so more than a thousand times a second on a busy system. Who says that token2 will even get a chance to run during that iteration?

If token2 needs to complete some critical operation before closing down, then it needs to enforce that invariant, not leave it up to potentially racy logic.

Contributor

rrichardson commented Jun 3, 2015

When a poll() waits and completes is effectively arbitrary, but it probably does so more than a thousand times a second on a busy system. Who says that token2 will even get a chance to run during that iteration?

If token2 needs to complete some critical operation before closing down, then it needs to enforce that invariant, not leave it up to potentially racy logic.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 3, 2015

Contributor

That said, I guess it's not a huge amount of overhead to have such a final() callback, who am I to judge someone's kinks. If that's what they're into, we can supply.

OK.

My proposal:

  1. remove readable and writeable, change to poll_event() (I think event() is too generic, since there are still other handlers, notify and timeout)
  2. Coalesce all events into a single mask per file descriptor for every poll() run.
  3. Add a final() function. Maybe pass in the number of events handled on this run (or some other interesting statistics for reporting)
Contributor

rrichardson commented Jun 3, 2015

That said, I guess it's not a huge amount of overhead to have such a final() callback, who am I to judge someone's kinks. If that's what they're into, we can supply.

OK.

My proposal:

  1. remove readable and writeable, change to poll_event() (I think event() is too generic, since there are still other handlers, notify and timeout)
  2. Coalesce all events into a single mask per file descriptor for every poll() run.
  3. Add a final() function. Maybe pass in the number of events handled on this run (or some other interesting statistics for reporting)
@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jun 3, 2015

Owner

@rrichardson I'm fine w/ that proposal. The specific coalescing strategy doesn't have to be optimal yet since I believe the only officially supported kqueue system as of today is OS X. I am confident enough that it is possible to implement a very low overhead coalescing strategy in the future. It would be nice if the implementation didn't allocate though.

Now, bike shedding wise... what about ready instead of poll_event()?

Owner

carllerche commented Jun 3, 2015

@rrichardson I'm fine w/ that proposal. The specific coalescing strategy doesn't have to be optimal yet since I believe the only officially supported kqueue system as of today is OS X. I am confident enough that it is possible to implement a very low overhead coalescing strategy in the future. It would be nice if the implementation didn't allocate though.

Now, bike shedding wise... what about ready instead of poll_event()?

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 3, 2015

Contributor

I like ready().

On Wed, Jun 3, 2015, 1:07 PM Carl Lerche notifications@github.com wrote:

@rrichardson https://github.com/rrichardson I'm fine w/ that proposal.
The specific coalescing strategy doesn't have to be optimal yet since I
believe the only officially supported kqueue system as of today is OS X. I
am confident enough that it is possible to implement a very low overhead
coalescing strategy in the future. It would be nice if the implementation
didn't allocate though.

Now, bike shedding wise... what about ready instead of poll_event()?


Reply to this email directly or view it on GitHub
#184 (comment).

Contributor

rrichardson commented Jun 3, 2015

I like ready().

On Wed, Jun 3, 2015, 1:07 PM Carl Lerche notifications@github.com wrote:

@rrichardson https://github.com/rrichardson I'm fine w/ that proposal.
The specific coalescing strategy doesn't have to be optimal yet since I
believe the only officially supported kqueue system as of today is OS X. I
am confident enough that it is possible to implement a very low overhead
coalescing strategy in the future. It would be nice if the implementation
didn't allocate though.

Now, bike shedding wise... what about ready instead of poll_event()?


Reply to this email directly or view it on GitHub
#184 (comment).

@carllerche carllerche added this to the v0.4 Release milestone Jun 3, 2015

@reem

This comment has been minimized.

Show comment
Hide comment
@reem

reem Jun 3, 2015

Contributor

If we have 3 in @rrichardson's proposal, do we actually need to do 2? That decision could be left to user code, so we don't imply the overhead of coalescing on kqueue.

Contributor

reem commented Jun 3, 2015

If we have 3 in @rrichardson's proposal, do we actually need to do 2? That decision could be left to user code, so we don't imply the overhead of coalescing on kqueue.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 3, 2015

Contributor

2 is mostly for ergonomics and the principle of last surprise. As long as
we pass hints to each ready() call, there is no actual need. It just makes
EventLoop behave the same regardless of whether it's over kevent or epoll.
Otherwise the use customer code has to be built to anticipate both models.

On Wed, Jun 3, 2015, 3:17 PM Jonathan Reem notifications@github.com wrote:

If we have 3 in @rrichardson https://github.com/rrichardson's proposal,
do we actually need to do 2? That decision could be left to user code, so
we don't imply the overhead of coalescing on kqueue.


Reply to this email directly or view it on GitHub
#184 (comment).

Contributor

rrichardson commented Jun 3, 2015

2 is mostly for ergonomics and the principle of last surprise. As long as
we pass hints to each ready() call, there is no actual need. It just makes
EventLoop behave the same regardless of whether it's over kevent or epoll.
Otherwise the use customer code has to be built to anticipate both models.

On Wed, Jun 3, 2015, 3:17 PM Jonathan Reem notifications@github.com wrote:

If we have 3 in @rrichardson https://github.com/rrichardson's proposal,
do we actually need to do 2? That decision could be left to user code, so
we don't imply the overhead of coalescing on kqueue.


Reply to this email directly or view it on GitHub
#184 (comment).

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 3, 2015

Contributor

That should read principle of least surprise grumble mobile

Contributor

rrichardson commented Jun 3, 2015

That should read principle of least surprise grumble mobile

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jun 3, 2015

Owner

Also, I think the strategy that I outlined above should be minimal overhead and could be tuned to be close to 0

Owner

carllerche commented Jun 3, 2015

Also, I think the strategy that I outlined above should be minimal overhead and could be tuned to be close to 0

@reem

This comment has been minimized.

Show comment
Hide comment
@reem

reem Jun 3, 2015

Contributor

If there's no noticeable performance cost to 2 then I am for it, it certainly does make the programming model simpler.

Contributor

reem commented Jun 3, 2015

If there's no noticeable performance cost to 2 then I am for it, it certainly does make the programming model simpler.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jun 3, 2015

Owner

@reem the strategy would be what was outlined here. Thoughts?

Owner

carllerche commented Jun 3, 2015

@reem the strategy would be what was outlined here. Thoughts?

@reem

This comment has been minimized.

Show comment
Hide comment
@reem

reem Jun 3, 2015

Contributor

@carllerche the std HashMap allows you to define your own hasher and hashing algorithm for your key type and has a very optimized robin hood implementation and allocation strategy, so we could probably use that rather than adding complexity to mio if there is some way to prevent it from resizing. Other than that it sounds fine, hard to say more without looking at real perf numbers.

Contributor

reem commented Jun 3, 2015

@carllerche the std HashMap allows you to define your own hasher and hashing algorithm for your key type and has a very optimized robin hood implementation and allocation strategy, so we could probably use that rather than adding complexity to mio if there is some way to prevent it from resizing. Other than that it sounds fine, hard to say more without looking at real perf numbers.

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 3, 2015

Contributor

Yea, being able to preallocate slots sized to a safe value for the max
number of events returned by kqueue is also very helpful. It would be nice
to to get insight into the stats of the hashmap to know how effective the
sizing is in terms of rehashing etc

On Wed, Jun 3, 2015, 5:35 PM Jonathan Reem notifications@github.com wrote:

@carllerche https://github.com/carllerche the std HashMap allows you to
define your own hasher and hashing algorithm for your key type and has a
very optimized robin hood implementation and allocation strategy, so we
could probably use that rather than adding complexity to mio if there is
some way to prevent it from resizing. Other than that it sounds fine, hard
to say more without looking at real perf numbers.


Reply to this email directly or view it on GitHub
#184 (comment).

Contributor

rrichardson commented Jun 3, 2015

Yea, being able to preallocate slots sized to a safe value for the max
number of events returned by kqueue is also very helpful. It would be nice
to to get insight into the stats of the hashmap to know how effective the
sizing is in terms of rehashing etc

On Wed, Jun 3, 2015, 5:35 PM Jonathan Reem notifications@github.com wrote:

@carllerche https://github.com/carllerche the std HashMap allows you to
define your own hasher and hashing algorithm for your key type and has a
very optimized robin hood implementation and allocation strategy, so we
could probably use that rather than adding complexity to mio if there is
some way to prevent it from resizing. Other than that it sounds fine, hard
to say more without looking at real perf numbers.


Reply to this email directly or view it on GitHub
#184 (comment).

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Jun 6, 2015

Contributor

added #194 to address this.

Contributor

rrichardson commented Jun 6, 2015

added #194 to address this.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jul 7, 2015

Owner

An initial pass of this is almost ready to merge: https://github.com/carllerche/mio/tree/coalesce-events

Owner

carllerche commented Jul 7, 2015

An initial pass of this is almost ready to merge: https://github.com/carllerche/mio/tree/coalesce-events

carllerche added a commit that referenced this issue Jul 7, 2015

Coalesce readable & writable into `ready` event
Having separate readable & writable events makes cleanly closing the socket
and freeing up resources a bit tricky. Instead, a single `ready` event is
used. The guarantees are that the event will be invoked only a single time per
socket per event loop tick.

Fixes #184

carllerche added a commit that referenced this issue Jul 7, 2015

Coalesce readable & writable into `ready` event
Having separate readable & writable events makes cleanly closing the socket
and freeing up resources a bit tricky. Instead, a single `ready` event is
used. The guarantees are that the event will be invoked only a single time per
socket per event loop tick.

Fixes #184

@carllerche carllerche closed this in #207 Jul 8, 2015

@arthurprs

This comment has been minimized.

Show comment
Hide comment
@arthurprs

arthurprs Jul 14, 2015

Contributor

Thank you so much @carllerche!!!

We should probably use a faster hash in the HashMap though.

Contributor

arthurprs commented Jul 14, 2015

Thank you so much @carllerche!!!

We should probably use a faster hash in the HashMap though.

@arthurprs

This comment has been minimized.

Show comment
Hide comment
@arthurprs

arthurprs Jul 14, 2015

Contributor

For keys <= 8 bytes it's really hard to beat FNV as it's super simple and small (thus inlined).

Contributor

arthurprs commented Jul 14, 2015

For keys <= 8 bytes it's really hard to beat FNV as it's super simple and small (thus inlined).

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jul 15, 2015

Owner

@arthurprs This is definitely an initial pass implementation. I'm thinking that I will probably just use a mask as the hash function. Given that tokens tend to be sequential (when used with a slab), this should have a fairly good collision rate. Also, allocating the map capacity to the max # of connections will completely avoid collisions at the expense of memory.

Owner

carllerche commented Jul 15, 2015

@arthurprs This is definitely an initial pass implementation. I'm thinking that I will probably just use a mask as the hash function. Given that tokens tend to be sequential (when used with a slab), this should have a fairly good collision rate. Also, allocating the map capacity to the max # of connections will completely avoid collisions at the expense of memory.

@arthurprs

This comment has been minimized.

Show comment
Hide comment
@arthurprs

arthurprs Jul 16, 2015

Contributor

Yeah, with the robin-hood under the hood we can probably get away just returning the integer (a bit of overallocation won't hurt either).

Heck, python gets away with it (hashing integers as themselves) using a more basic open addressing and 67% resize policy.

Contributor

arthurprs commented Jul 16, 2015

Yeah, with the robin-hood under the hood we can probably get away just returning the integer (a bit of overallocation won't hurt either).

Heck, python gets away with it (hashing integers as themselves) using a more basic open addressing and 67% resize policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment