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

Please make tokens consistent #41

Closed
dtantsur opened this issue Nov 1, 2014 · 4 comments
Closed

Please make tokens consistent #41

dtantsur opened this issue Nov 1, 2014 · 4 comments

Comments

@dtantsur
Copy link
Contributor

dtantsur commented Nov 1, 2014

Before attempting to write generic token patch, I looked through git history and realised that you've actually dropped this capability in 49e99b3. While I'm not feeling confident at all about this decision and hope you'll revisit it one day, currently I suggest making it consistent and dropping generic T argument from Timer and EventLoop. It's of little use without support for tokens in the events and it's extremely confusing. It required me too look through the whole source code (and git history!) to finally understand, why EventLoop accepts type argument like Token, but then uses unrelated type Token that is just uint (and IMO should be dropped in favor of just using uint).

On a related side, I saw discussion in the Rust upstream about using uint and int, can't find link now, but it was considered a bad practice using int and uint in most cases. I think it's better to use specific sized types.

TL;DR I suggest dropping T from Timer and EventLoop and making all tokens just uXX where XX is a size, possibly platform-dependent.

@ayosec
Copy link

ayosec commented Nov 1, 2014

I guess that mio has to use platform-dependent integers because the value of the token is stored in the internal data of epoll/kqueue, so there is no way to use a generic value, bigger than 32/64 bits.

Maybe, you can use a (really ugly) hack using a box and std::mem::transmute.

let my_data = box MyData::new(...);
let my_data_token: uint = unsafe { mem::transmute(my_data) };

@dtantsur
Copy link
Contributor Author

dtantsur commented Nov 1, 2014

  1. Before mentioned patch mio was using trait Token that allowed converting to uint, so no problem here.
  2. Also, uint may or may not what you think it is. IIRC it's defined in Rust as minimum number enough to hold a pointer, which is not how it is defined in C/C++ - see e.g. discussion in https://github.com/1fish2/Rust-rfcs/blob/int-index-for-portability/active/0000-intindex.md

@carllerche
Copy link
Member

The reason that I moved away from the generic for socket tokens was because

a) Like @ayosec mentioned, it is stored by epoll / kqueue and must be no bigger than a pointer size. As of now in the rust reference manual, uint is defined as the equivalent to C's uintptr_t, which is what is needed. If rust decides to change the meaning of uint, then Token will need to be updated to explicitly mean pointer sized.

b) There is no way to run drop functions correctly and any trait for the socket token would have to implement Copy.

Given these two requirements (and the inability to set size bounds on traits), it didn't seem worth making the socket token generic. I expect the main usage pattern will be to store of state in some structure and then pass the token to mio. As to why Token vs. just going w/ a raw number? I prefer making types specific to the use case at hand, but that might be personal preference.

Now, regarding why I didn't use Token across the board, the limitation is quite annoying, and I didn't see a reason to arbitrarily limit the the API. I completely disagree that it is of little use. I am finding it quite useful. Avoiding the double lookup when possible has benefits.

@wycats
Copy link
Contributor

wycats commented Nov 2, 2014

There is no way to run drop functions correctly and any trait for the socket token would have to implement Copy.

Just to clarify, I believe what @carllerche is saying is that even though epoll removes closed FDs from the set, we have not found a reliable way to learn when an FD has been closed. This means that we have no way to drop tokens when the associated FD is no longer active, which will cause bad memory leaks.

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

4 participants