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

Iterator for Poller #43

Closed
wants to merge 1 commit into from
Closed

Iterator for Poller #43

wants to merge 1 commit into from

Conversation

dtantsur
Copy link
Contributor

@dtantsur dtantsur commented Nov 5, 2014

While switching EventLoop to iterator instead of indexing, I spotted a
potencial problem: io_process expects list of events not to change during it's
execution, but self is passed as &mut to the handler, making it possible (at
least in theory) to break it. After switch to an iterator, this issue is caught
by borrow checker actually. I switched the code to collect()ing all events into
a Vec as a workaround for now and looking forward to better ideas.

@carllerche
Copy link
Member

The reason for the existing code is to be able to avoid allocations at runtime (and just have the Events list stack allocated).

It would be nice to use an Iterator, but requiring a vec allocation each iteration is not worth it. I don't have any better alternative ideas, but I haven't spent too much time thinking about it yet.

While switching EventLoop to iterator instead of indexing, I spotted a
potencial problem: io_process expects list of events not to change during it's
execution, but self is passed as &mut to the handler, making it possible (at
least in theory) to break it. After switch to an iterator, this issue is caught
by borrow checker actually. I switched the code to collect()ing all events into
a Vec as a workaround for now and looking forward to better ideas.
@dtantsur
Copy link
Contributor Author

dtantsur commented Nov 5, 2014

I see benefits of both approaches, so I've withdrawn change to event_loop and left only adding iter(). However, I still thing that giving handler ability to break (potentially) event loop is kind of a bug.

I'll also rethink it a bit more later.

@dtantsur
Copy link
Contributor Author

@carllerche any opinion on the shorter patch?

@carllerche
Copy link
Member

This seems like it is fine. Quick question though, are you using Poll directly? I think there is some discussion about whether or not Poll is needed vs. just rolling it into EventLoop. I don't know if you have thoughts about that.

@dtantsur
Copy link
Contributor Author

I was thinking about using it, yes. EventLoop is too heavyweight if you need e.g. to multiplex 2 sockets for reading :)

@carllerche
Copy link
Member

Sorry for the delay. I ended up merging this at abbe04d

@carllerche carllerche closed this Jan 5, 2015
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

Successfully merging this pull request may close these issues.

None yet

2 participants