Skip to content
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
Closed

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

dpc opened this issue Jul 25, 2015 · 27 comments

Comments

@dpc
Copy link
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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
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.

@dpc
Copy link
Contributor Author

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
Copy link
Contributor

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

@carllerche
Copy link
Member

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

@tailhook
Copy link
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

@carllerche
Copy link
Member

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
Copy link
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.

@dpc
Copy link
Contributor Author

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
Copy link
Contributor

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

@dpc
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Member

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.

@rrichardson
Copy link
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.

@dwrensha
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

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

@tailhook
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
The `Handler::tick()` method is called at the beginning of each event
loop tick.

Fixes tokio-rs#219
hjr3 added a commit to hjr3/mio that referenced this issue Aug 21, 2015
The `Handler::tick()` method is called at the end of each event
loop tick.

Fixes tokio-rs#219
hjr3 added a commit to hjr3/mio that referenced this issue Aug 21, 2015
The `Handler::tick()` method is called at the end of each event
loop tick.

Fixes tokio-rs#219
hjr3 added a commit to hjr3/mob that referenced this issue Aug 22, 2015
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 tokio-rs/mio#219 for more details.

Fixes #1
carllerche pushed a commit that referenced this issue Aug 31, 2015
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
* 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. tokio-rs/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
* 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. tokio-rs/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
Copy link
Contributor

hjr3 commented Sep 29, 2015

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

@tailhook
Copy link
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.

@hjr3
Copy link
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
Copy link
Member

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

@hjr3
Copy link
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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants