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

Does deregister clear events already pending delivery? Should it? #219

Closed
dpc opened this Issue Jul 25, 2015 · 27 comments

Comments

Projects
None yet
7 participants
@dpc
Contributor

dpc commented Jul 25, 2015

Let's say I have two event sources registered, both of them fired at the same time and landed in pending events list for current iteration.

While handling the first event, I do deregister on the other one. Will I still get it? It seems to me that I will, which is very inconvenient, as it makes hard (impossible?) to safely deregister events other than the one that just fired.

A spin of the above case is: What if I do deregister and right after used register_opt on different event source, using the same token.

Another spinoff is: What if I do reregister using different interest.

If I am not missing anything, it seems to me that either: the end of each event delivery iteration should have a notification, during which calls like deregister and reregister can be safely used, or deregister and reregister should make sure to remove any events that are not to be delivered anymore.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jul 27, 2015

Owner

You have a chance of receiving the event.

I think having Handler::tick() that is called at the end of the iteration would be fine.

Owner

carllerche commented Jul 27, 2015

You have a chance of receiving the event.

I think having Handler::tick() that is called at the end of the iteration would be fine.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Jul 27, 2015

Contributor

That would work for mioco. Though if mio could remove such events efficiently enough on it's own it would be even nicer.

Contributor

dpc commented Jul 27, 2015

That would work for mioco. Though if mio could remove such events efficiently enough on it's own it would be even nicer.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jul 27, 2015

Owner

The best way to do it would require an array scan of buffered events when deregistering tokens that are not the current one being processed. I'm unsure if this is worth the overhead vs. adding a Handler::tick()

cc @rrichardson

Owner

carllerche commented Jul 27, 2015

The best way to do it would require an array scan of buffered events when deregistering tokens that are not the current one being processed. I'm unsure if this is worth the overhead vs. adding a Handler::tick()

cc @rrichardson

@arthurprs

This comment has been minimized.

Show comment
Hide comment
@arthurprs

arthurprs Jul 30, 2015

Contributor

I like the Handler::tick() alternative more, otherwise you end up with loads of de/register around, and array scanning every time might not be cheap.

Contributor

arthurprs commented Jul 30, 2015

I like the Handler::tick() alternative more, otherwise you end up with loads of de/register around, and array scanning every time might not be cheap.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Jul 31, 2015

Contributor

I'm hopping for no tick(). If the coalesced events are already in cheap-lookup HashMap, than the overhead would be one lookup per de/re-register. With tick() user needs to put the entries to be deleted into something (dynamic), and can't write the code with an assertion that only events that are actually registered, can be delivered. I think it might be condition that is easy to overlook, and forget about. And noone likes dealing with servers that hang/crash once in a blue moon.

Contributor

dpc commented Jul 31, 2015

I'm hopping for no tick(). If the coalesced events are already in cheap-lookup HashMap, than the overhead would be one lookup per de/re-register. With tick() user needs to put the entries to be deleted into something (dynamic), and can't write the code with an assertion that only events that are actually registered, can be delivered. I think it might be condition that is easy to overlook, and forget about. And noone likes dealing with servers that hang/crash once in a blue moon.

@arthurprs

This comment has been minimized.

Show comment
Hide comment
@arthurprs

arthurprs Jul 31, 2015

Contributor

@dpc that's true for kqueue, but I guess we can extend it to epool too in order to implement this.

Contributor

arthurprs commented Jul 31, 2015

@dpc that's true for kqueue, but I guess we can extend it to epool too in order to implement this.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jul 31, 2015

Owner

I am currently leaning against adding any overhead in the epoll case. Hoping more people weigh in though.

Owner

carllerche commented Jul 31, 2015

I am currently leaning against adding any overhead in the epoll case. Hoping more people weigh in though.

@tailhook

This comment has been minimized.

Show comment
Hide comment
@tailhook

tailhook Jul 31, 2015

Contributor

With tick() user needs to put the entries to be deleted into something (dynamic)

Well I'm not sure it's even possible in some sane way. Let's me try to explain...

  1. For deregister you need to put socket into deregister queue. Deregister queue probably contains Box because there are many kinds of sockets.
  2. It's ok to put socket if you closing it. It will be just popped of the queue, deregistered and closed/freed.
  3. In case you need to temporarily deregister, you can't just put Evented trait. You probably need to put a token. Which you need to resolve into socket e.g. via Slab, which usually resolves to one of different kinds of state machines, which you need to get socket from with some code specific for this state machine.

And when using Slab for tokens, as @dpc already said, you can't just deregister and deallocate and tolerate spurious events, because token might get reused in this loop iteration.

So it's kinda ugly. And I believe it should be fixed by mio. In case you can tolerate that and want "raw" performance, there is always an option to use Poll instead of EventLoop

Contributor

tailhook commented Jul 31, 2015

With tick() user needs to put the entries to be deleted into something (dynamic)

Well I'm not sure it's even possible in some sane way. Let's me try to explain...

  1. For deregister you need to put socket into deregister queue. Deregister queue probably contains Box because there are many kinds of sockets.
  2. It's ok to put socket if you closing it. It will be just popped of the queue, deregistered and closed/freed.
  3. In case you need to temporarily deregister, you can't just put Evented trait. You probably need to put a token. Which you need to resolve into socket e.g. via Slab, which usually resolves to one of different kinds of state machines, which you need to get socket from with some code specific for this state machine.

And when using Slab for tokens, as @dpc already said, you can't just deregister and deallocate and tolerate spurious events, because token might get reused in this loop iteration.

So it's kinda ugly. And I believe it should be fixed by mio. In case you can tolerate that and want "raw" performance, there is always an option to use Poll instead of EventLoop

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Jul 31, 2015

Owner

So, I haven't hit the issue in question yet, so I need some help trying to understand.

For cases where sockets aren't interrelated, there isn't an issue. The EventLoop won't produce any further events with the token.

For cases where multiple sockets are interrelated, say a proxy, where if one end closes, you want to close both ends. The issue seems to be that the tokens for both sockets are "released" back to the pool, thus any further notifications using the token will be problematic.

Having deregister guarantee that no further events happen w/ the token is not actually enough, because it doesn't handle the case where the socket is closed w/o deregister being called.

@tailhook I don't understand why point #3 is a problem. If you temporarily deregister a socket, you aren't releasing the token back into the pool, so there will be no token conflict.

Owner

carllerche commented Jul 31, 2015

So, I haven't hit the issue in question yet, so I need some help trying to understand.

For cases where sockets aren't interrelated, there isn't an issue. The EventLoop won't produce any further events with the token.

For cases where multiple sockets are interrelated, say a proxy, where if one end closes, you want to close both ends. The issue seems to be that the tokens for both sockets are "released" back to the pool, thus any further notifications using the token will be problematic.

Having deregister guarantee that no further events happen w/ the token is not actually enough, because it doesn't handle the case where the socket is closed w/o deregister being called.

@tailhook I don't understand why point #3 is a problem. If you temporarily deregister a socket, you aren't releasing the token back into the pool, so there will be no token conflict.

@tailhook

This comment has been minimized.

Show comment
Hide comment
@tailhook

tailhook Jul 31, 2015

Contributor

@tailhook I don't understand why point #3 is a problem. If you temporarily deregister a socket, you aren't releasing the token back into the pool, so there will be no token conflict.

Ah, okay, I just need to have different deregister code path for temporary and final deregister. And also ignore spurious events. It's not too simple, but may work.

Contributor

tailhook commented Jul 31, 2015

@tailhook I don't understand why point #3 is a problem. If you temporarily deregister a socket, you aren't releasing the token back into the pool, so there will be no token conflict.

Ah, okay, I just need to have different deregister code path for temporary and final deregister. And also ignore spurious events. It's not too simple, but may work.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Jul 31, 2015

Contributor

@carllerche , I'm not sure what do you mean in "Having deregister guarantee that no further events happen w/ the token is not actually enough, " paragraph.

The problem is with sockets being interrelated, yes. But the root of the problem is that deregister does not currently guarantee that "no further events happen w/ the token".

Because of the fact that that events are delivered in groups, if during processing of one event in the current group the interest in the following events in that group can be removed. This can happen in two cases: with deregister or reregister.

The core proposition is: "mio must guarantee that it will never deliver any event that the Handler is actively waiting for in a given time".

Contributor

dpc commented Jul 31, 2015

@carllerche , I'm not sure what do you mean in "Having deregister guarantee that no further events happen w/ the token is not actually enough, " paragraph.

The problem is with sockets being interrelated, yes. But the root of the problem is that deregister does not currently guarantee that "no further events happen w/ the token".

Because of the fact that that events are delivered in groups, if during processing of one event in the current group the interest in the following events in that group can be removed. This can happen in two cases: with deregister or reregister.

The core proposition is: "mio must guarantee that it will never deliver any event that the Handler is actively waiting for in a given time".

@arthurprs

This comment has been minimized.

Show comment
Hide comment
@arthurprs

arthurprs Aug 1, 2015

Contributor

This might be problematic when using edge triggered events, or not?

Contributor

arthurprs commented Aug 1, 2015

This might be problematic when using edge triggered events, or not?

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Aug 2, 2015

Contributor

I don't see why. My understanding is: "edge triggered" means that if the condition generating even was deasserted, the event will be still generated. But this issue is about what Handler is expecting. If Handler deregistered from certain events, it should not receive them.

Contributor

dpc commented Aug 2, 2015

I don't see why. My understanding is: "edge triggered" means that if the condition generating even was deasserted, the event will be still generated. But this issue is about what Handler is expecting. If Handler deregistered from certain events, it should not receive them.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Aug 3, 2015

Owner

@dpc My point is, "closing" a socket is also "deregisters" the socket. However, since this entirely happens in the kernel, there is no way for mio to purge any pending events.

Owner

carllerche commented Aug 3, 2015

@dpc My point is, "closing" a socket is also "deregisters" the socket. However, since this entirely happens in the kernel, there is no way for mio to purge any pending events.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Aug 3, 2015

Owner

So, either the two options are:

  • There may be spurious events for a deregistered or closed socket until the event loop ticks.
  • You must call deregister for all sockets that are being closed, which will add overhead.
Owner

carllerche commented Aug 3, 2015

So, either the two options are:

  • There may be spurious events for a deregistered or closed socket until the event loop ticks.
  • You must call deregister for all sockets that are being closed, which will add overhead.

@dpc dpc referenced this issue Aug 4, 2015

Closed

Growable slab. #8

@rrichardson

This comment has been minimized.

Show comment
Hide comment
@rrichardson

rrichardson Aug 4, 2015

Contributor

It is tempting to use mio as both a state machine and general event management system. I would recommend against this. One reason is that, despite our efforts to the contrary, you might get different behavior on different platforms, since it is the OS kernel that is generating the events. I would recommend in addition to de-registering descriptors on an event, that you also update some internal state and use that state to dictate your actions when handling events. This is the only way to be sure that you'll get consistent performance.

tl;dr Changing mio to purge events would be too much effort, both in developer effort and cpu cycles to support a mechanism that shouldn't be used. One should not be using deregister as a means of influencing control flow in their system.

Contributor

rrichardson commented Aug 4, 2015

It is tempting to use mio as both a state machine and general event management system. I would recommend against this. One reason is that, despite our efforts to the contrary, you might get different behavior on different platforms, since it is the OS kernel that is generating the events. I would recommend in addition to de-registering descriptors on an event, that you also update some internal state and use that state to dictate your actions when handling events. This is the only way to be sure that you'll get consistent performance.

tl;dr Changing mio to purge events would be too much effort, both in developer effort and cpu cycles to support a mechanism that shouldn't be used. One should not be using deregister as a means of influencing control flow in their system.

@dwrensha

This comment has been minimized.

Show comment
Hide comment
@dwrensha

dwrensha Aug 4, 2015

Contributor

@rrichardson +1

My opinion is that any registration or de-registration should only take place between calls to mio::EventLoop::run_once() (or inside of mio::Handler::tick() if you're using mio::EventLoop::run()). The invariants that hold during mio::Handler::ready() are weaker and more confusing (and perhaps less platform-consistent) than the invariants that hold between ticks.

Contributor

dwrensha commented Aug 4, 2015

@rrichardson +1

My opinion is that any registration or de-registration should only take place between calls to mio::EventLoop::run_once() (or inside of mio::Handler::tick() if you're using mio::EventLoop::run()). The invariants that hold during mio::Handler::ready() are weaker and more confusing (and perhaps less platform-consistent) than the invariants that hold between ticks.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Aug 4, 2015

Contributor

So the tick() then? It's OK with me, but it needs to be well documented (all the things that must not be assumed).

Contributor

dpc commented Aug 4, 2015

So the tick() then? It's OK with me, but it needs to be well documented (all the things that must not be assumed).

@arthurprs

This comment has been minimized.

Show comment
Hide comment
@arthurprs

arthurprs Aug 4, 2015

Contributor

If we have a tick() method we can actually remove the event aggregation machinery from KQueue, right?

Contributor

arthurprs commented Aug 4, 2015

If we have a tick() method we can actually remove the event aggregation machinery from KQueue, right?

@tailhook

This comment has been minimized.

Show comment
Hide comment
@tailhook

tailhook Aug 4, 2015

Contributor

I would recommend in addition to de-registering descriptors on an event, that you also update some internal state and use that state to dictate your actions when handling events

protip: if you keep internal state anyway it's better use use edge-triggered mode because that allows to avoid all reregister calls too.

Contributor

tailhook commented Aug 4, 2015

I would recommend in addition to de-registering descriptors on an event, that you also update some internal state and use that state to dictate your actions when handling events

protip: if you keep internal state anyway it's better use use edge-triggered mode because that allows to avoid all reregister calls too.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Aug 4, 2015

Contributor

I wonder if it would be possible to write a layer library on top of mio that would have pretty much the same API, but did this state tracking internally. Or maybe mio could have a "well behaved" adaptor structs for those willing to pay the overhead price.

Contributor

dpc commented Aug 4, 2015

I wonder if it would be possible to write a layer library on top of mio that would have pretty much the same API, but did this state tracking internally. Or maybe mio could have a "well behaved" adaptor structs for those willing to pay the overhead price.

@dpc

This comment has been minimized.

Show comment
Hide comment
@dpc

dpc Aug 8, 2015

Contributor

I might attempt writing such a functionality in mio itself. Would you be OK if all the overhead we discuss here was a compile time feature?

It's just in mioco spurious events are ruining select functionality. Ways to overcome this are:

  • complicate select API to tolerate cases in which event source said it was ready, but it lied; logic is simple, but the resulting API hidoeus,
  • write a lot of checks to catch all cases of spurious events,
  • custom logic handling registration state change,
  • use only the first event for a given coroutine from the whole group between two tick()-s, as it's the only event a coroutine handler can really trust (this is my preferred solution so far).

So I think it might be worth it to take a tiny overhead (one fast lookup on every reregister and deregister) than complicate your code and risk having software that in certain situations experiences weird hiccups.

The feature would be making mio well-behaved meaning guarantee no spurious events, as long as deregister is always used.

The benefit of compile time feature is that people experiencing any issues could quickly check if that might be a cause. And those sure that their code handles this conditions just right, can get a pure performance version.

Contributor

dpc commented Aug 8, 2015

I might attempt writing such a functionality in mio itself. Would you be OK if all the overhead we discuss here was a compile time feature?

It's just in mioco spurious events are ruining select functionality. Ways to overcome this are:

  • complicate select API to tolerate cases in which event source said it was ready, but it lied; logic is simple, but the resulting API hidoeus,
  • write a lot of checks to catch all cases of spurious events,
  • custom logic handling registration state change,
  • use only the first event for a given coroutine from the whole group between two tick()-s, as it's the only event a coroutine handler can really trust (this is my preferred solution so far).

So I think it might be worth it to take a tiny overhead (one fast lookup on every reregister and deregister) than complicate your code and risk having software that in certain situations experiences weird hiccups.

The feature would be making mio well-behaved meaning guarantee no spurious events, as long as deregister is always used.

The benefit of compile time feature is that people experiencing any issues could quickly check if that might be a cause. And those sure that their code handles this conditions just right, can get a pure performance version.

hjr3 added a commit to hjr3/mio that referenced this issue Aug 20, 2015

Implement Handler::tick()
The `Handler::tick()` method is called at the beginning of each event
loop tick.

Fixes #219

hjr3 added a commit to hjr3/mio that referenced this issue Aug 21, 2015

Implement Handler::tick()
The `Handler::tick()` method is called at the end of each event
loop tick.

Fixes #219

hjr3 added a commit to hjr3/mio that referenced this issue Aug 21, 2015

Implement Handler::tick()
The `Handler::tick()` method is called at the end of each event
loop tick.

Fixes #219

@carllerche carllerche closed this in #231 Aug 21, 2015

hjr3 added a commit to hjr3/mob that referenced this issue Aug 22, 2015

Do not try to remove a connection the slab twice
A single event loop tick may contain multiple events for a token. If the
connection is reset on the first event, subsequent events will still try
to access that token/connection during the event loop tick. The strategy
now is to mark a connection as reset and remove them only when the event
loop tick is finished.

See carllerche/mio#219 for more details.

Fixes #1

carllerche added a commit that referenced this issue Aug 31, 2015

Implement Handler::tick()
The `Handler::tick()` method is called at the end of each event
loop tick.

Fixes #219

dpc added a commit to dpc/mioco.pre-0.9 that referenced this issue Sep 9, 2015

Change semantics somewhat.
* Make `select-*` explicitly warn about returning spurious events. It's
  just to complicated (impossible?) to guarantee never waking up on a
  spurious event. (see eg. carllerche/mio#219)
* Introduce `try_write()`/`try_read()`, to be used with `select-*`
* Rename and tweak operations on some `EventSource`-s, since this is
  breaking change anyway.
* Bump the version.

dpc added a commit to dpc/mioco.pre-0.9 that referenced this issue Sep 10, 2015

Change semantics somewhat.
* Make `select-*` explicitly warn about returning spurious events. It's
  just to complicated (impossible?) to guarantee never waking up on a
  spurious event. (see eg. carllerche/mio#219)
* Introduce `try_write()`/`try_read()`, to be used with `select-*`
* Rename and tweak operations on some `EventSource`-s, since this is
  breaking change anyway.
* Bump the version.
@hjr3

This comment has been minimized.

Show comment
Hide comment
@hjr3

hjr3 Sep 29, 2015

Contributor

@tailhook how would internal state + edge-triggered avoid any reregister calls?

Contributor

hjr3 commented Sep 29, 2015

@tailhook how would internal state + edge-triggered avoid any reregister calls?

@tailhook

This comment has been minimized.

Show comment
Hide comment
@tailhook

tailhook Sep 29, 2015

Contributor

@hjr3 you just always listen for reads and writes and keep internal state of whether your socket is still readable or writable. I mean if you want to pause reading, you don't to deregister (or anything actually). When you want to unpause reading, you look at internal state: if the socket was readable, read from the socket until EAGAIN (or TryRead::read returns Ok(None)). If it wasn't, wait until next edge-triggered read event.

Contributor

tailhook commented Sep 29, 2015

@hjr3 you just always listen for reads and writes and keep internal state of whether your socket is still readable or writable. I mean if you want to pause reading, you don't to deregister (or anything actually). When you want to unpause reading, you look at internal state: if the socket was readable, read from the socket until EAGAIN (or TryRead::read returns Ok(None)). If it wasn't, wait until next edge-triggered read event.

@hjr3

This comment has been minimized.

Show comment
Hide comment
@hjr3

hjr3 Sep 29, 2015

Contributor

@tailhook do you have an example of this? I thought edge-triggered meant the event loop will automatically deregister our connection when we receive a read/write event. Maybe we can discuss it more on irc. I am hjr3 in the mio channel as well.

Contributor

hjr3 commented Sep 29, 2015

@tailhook do you have an example of this? I thought edge-triggered meant the event loop will automatically deregister our connection when we receive a read/write event. Maybe we can discuss it more on irc. I am hjr3 in the mio channel as well.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Sep 29, 2015

Owner

edge + oneshot will disarm the FD after an event is fired.

Owner

carllerche commented Sep 29, 2015

edge + oneshot will disarm the FD after an event is fired.

@hjr3

This comment has been minimized.

Show comment
Hide comment
@hjr3

hjr3 Sep 29, 2015

Contributor

Ah, right. Thanks @carllerche

Contributor

hjr3 commented Sep 29, 2015

Ah, right. Thanks @carllerche

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