Use autoincrementing number for handler ids #105

dlrobertson · 2016-10-01T03:24:21Z

Use Uuid instead of i64 for the handler keys to ensure file descriptor number reuse doesn't cause errors, and make sure to close file descriptors.

NB: The scope of the PR has changed to change the handler id's from a platform specific i64 to an autoincrementing u64. macos has not yet been ported, and probably suffers from the same leak as the unix module did prior to this PR

vvuk · 2016-10-14T20:00:14Z

Instead of uuid, can we just have a i64 id that we just atomically increment when we need one? Would also mean no changes to anything but the platform backends.

dlrobertson · 2016-10-17T22:42:24Z

Valid point. We could do that, but imo if we did that we should definitely implement some sort of id reuse. The creation of the id reuse algorithm could take as much effort as switching Uuid, depending on the route taken.

vvuk · 2016-10-18T13:27:35Z

Why do we need id reuse? If a program allocated a new id every millisecond, it would take ~585 million years to exaust 2^64. I think that's a pretty good definition of "someone else's problem" ;) (1 per ns gives us 585 years, which is slightly more concerning, but only slightly.)

dlrobertson · 2016-10-18T19:20:02Z

Haha very vary true and a very good point... I though about that, and AFAIK you'd have to exhaust ((2 ^ 32) - 1) due to the use of i64 and the two's complement, so you'd end up with ~292 years which from what I've heard is about as long as it would take to generate a duplicate Uuid (from what I've read on the internets). If we switched to u64 for the id we'd be at ~585 to cause overflow. But at that point, the chances of generating a duplicate or causing overflow is pretty low.

If the consensus is to use an auto incrementing i64 I'm 100% okay with that. I'm just hyper paranoid about writing code that allows for overflow, but I suppose that is what checked_add is for.

antrik · 2016-10-19T22:19:20Z

It's 2^63-1 till integer overflow -- actual collision would indeed be exactly 2^64 I believe. You should be able to avoid the overflow, to squeeze out that last bit (literally), by using u64 internally, and casting to i64 as necessary.

(For production builds, it should produce the exact same machine code I think?)

Either way, I think it will be more efficient and less invasive than using UUIDs.

I'd say the actual FD closing should probably go into a separate patch. (The ID change lays the groundwork by decoupling the handles from the FDs; while the main change builds on top of this to fix the leak AIUI...)

Changing the handling of r_id to use Option<> can probably also go into a separate patch before the main ID handling change, for easier review... Unless it's too much hassle to untangle it I guess.

dlrobertson · 2016-10-25T22:06:12Z

using u64 internally, and casting to i64 as necessary.

Good point. I'll use this in the update

I'd say the actual FD closing should probably go into a separate patch.

Np. Nothing git rebase can't handle.

Thanks @antrik and @vvuk! I'll have some time this weekend and push the update using u64.

bors-servo · 2016-10-25T23:50:02Z

☔ The latest upstream changes (presumably #102) made this pull request unmergeable. Please resolve the merge conflicts.

Use an Option<i64> for r_id instead of a plain i64, and use None instead of -1 to indicate the id does not exist in the handles.

dlrobertson · 2016-11-07T00:48:04Z

Updated to use an auto-incrementing u64

antrik

The code looks good :-) (Aside from minor optimisations -- see below.)

The PR title however doesn't fit any more; and perhaps more importantly, I'm a bit unhappy with the commit messages... The second commit in particular actually has three somewhat distinct changes: switching the type of the tokens returned by OsIpcReceiverSet.add()/select() to u64, to delay potential integer overflow; factoring out the counter code from the inprocess implementation into a generic Incrementor facility, so it can be reused by the other back-ends; and decoupling the tokens used in the unix implementation from the actual FDs, to allow closing the FDs without introducing race conditions due to FD reuse. While I don't feel strongly about actually splitting these into separate commits (I don't think it substantially affects readability in this case), I think it would help a lot at least to explicitly mention and explain each of these changes in the commit message...

The third commit could mention that the FDs are closed in select(); and that this is to avoid them leaking. Also might want to point out that this is safe now with the previous changes.

The first patch ironically has the most explicit commit message, although it's the simplest change :-) Still, could add that this is in preparation for changing the type to u64.

Sorry for picking on formalities like this. I'm just pedantic in general; and I really like a readable history :-) As usual, feel free to ignore these remarks if you think them silly...

On an unrelated note, it might be good to mention somewhere that the macos implementation probably suffers from a similar leak? Though maybe that would be better placed in the issue tracker...

antrik · 2016-11-07T20:51:55Z

src/platform/unix/mod.rs

+            for fd in hangups.iter() {
+                self.fdids.remove(fd);
+                unsafe {
+                    libc::close(*fd);


While I do understand that we need to delay purging the entries from pollfds, I can't think of any reason for delaying the FD closing?...

Also, I just realised that fdids doesn't really need to be a HashMap: you could just put the IDs in a positional vector parallel to pollfds. Or if you keep it a HashMap, you can drop the fdids entries immediately; and then you won't need hangups any more, as you can then just use fdids to check which entries need to be retained in pollfds. Not sure which is more efficient -- and with the mio patch hopefully getting merged soon, it's probably not worth spending too much thought on it... Just pick whatever seems easier I'd say :-)

Use an auto incrementing u64 value as the OsIpcReceiver's id within the OsIpcReceiverSet in the inprocess and unix modules. - Change the type returned by OsIpcReceiverSet select and add to u64, to delay overflow. - Factor out the implementation of an auto-incrementing id inprocess into os::Incrementor, to be used by other platforms. - Use the Incrementor in the unix module to decouple the id of the OsIpcReceiver from the file descriptor to avoid race conditions due to file descriptor reuse.

dlrobertson · 2016-11-08T01:20:40Z

Thanks again, great critiques! I clearly need to work on better commit messages 😄.

dlrobertson · 2016-11-08T01:27:21Z

src/platform/unix/mod.rs

-        }
+        // Avoid the use of `self` in closue to avoid errors due to the
+        // mutable borrow of `self` at line 483
+        let ids = &self.fdids;


The borrow checker has beaten me... I don't really like this, but this was the best workaround I could think of. If anyone can think of a better workaround, please let me know!

Either way... It's only temporary until the mio PR lands.

What happens if instead of doing an inline closure in line 483, you make a let-binding of a closure and pass it in as an argument to retain?

Good idea! but sadly no :(

I think I understand the situation; I don't understand though why you consider it a problem?...

It's not exactly uncommon to create temporary bindings to get around borrow conflicts. I'm not sure this even deserves a comment: it doesn't seem like an obscure situation that requires much of an explanation?...

(BTW, including line numbers in a comment is a bad idea: these are bound to change :-) )

On the other hand, I for my part would be tempted to add a comment explaining that the entries that got removed from fdids, because the channels got closed, also need to be purged from pollfds. (And perhaps even mention that we can only do that after we finished iterating pollfds?...) These are the kind of things that are probably not immediately obvious even to an experienced Rust programmer looking at this code for the first time.

(I am aware that the existing ipc-channel code doesn't have much in the manner of such explanations... Which I personally consider a pity.)

On a more nit-picky note, I'd drop the blank line after the temporary binding -- these two lines very much belong together. Also, I think it would actually be preferable to name the temporary as fdids as well...

(BTW, including line numbers in a comment is a bad idea: these are bound to change :-) )

Ha true! good call... Removed the line numbers

I also added a comment above the retain detailing the reason for the placement.

BTW, just for explanation (in case it isn't clear): the issue here isn't the borrow checker being dumb, but rather it's about the way how closures are handled. They automatically close over any binding used within the closure's code -- in this case the binding is self. The compiler doesn't try to figure out that you are actually accessing only one sub-field. If you want only the sub-field to be closed over, you have to create an explicit binding for it... Which is precisely what you did :-)

For unix OSes make sure to close the file descriptor on ChannelClosed. The file descriptors should be closed in select to avoid leaking fds after removal from pollfds (servo#96). After e06edbc this is safe and avoids the previously seen race condition due to file descriptor reuse.

antrik · 2016-11-10T15:00:37Z

Looks good to me :-) Now we just need someone to approve it...

emilio · 2016-11-10T15:31:01Z

@bors-servo r=antrik

bors-servo · 2016-11-10T15:31:07Z

📌 Commit 2e1081c has been approved by antrik

bors-servo · 2016-11-10T15:31:11Z

⌛ Testing commit 2e1081c with merge 357abb9...

Use autoincrementing number for handler ids Use `Uuid` instead of `i64` for the handler keys to ensure file descriptor number reuse doesn't cause errors, and make sure to close file descriptors. **NB:** The scope of the PR has changed to change the handler id's from a platform specific `i64` to an autoincrementing `u64`. `macos` has not yet been ported, and probably suffers from the same leak as the `unix` module did prior to this PR

bors-servo · 2016-11-10T15:40:40Z

☀️ Test successful - status-appveyor, status-travis

dlrobertson · 2016-11-11T01:22:13Z

Cool beans! I'll move back onto #94

emilio · 2016-11-11T01:31:37Z

Thanks for doing this work, it's awesome :)

On Thu, Nov 10, 2016 at 05:22:13PM -0800, Dan Robertson wrote:

Cool beans! I'll move back onto #94

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#105 (comment)

antrik mentioned this pull request Oct 19, 2016

Close the fd that we're removing from the pollfds list #96

Closed

Use Option over positive and negative numbers

11322a9

Use an Option<i64> for r_id instead of a plain i64, and use None instead of -1 to indicate the id does not exist in the handles.

dlrobertson force-pushed the use-uuid branch 2 times, most recently from eb72e53 to b8fdf19 Compare November 7, 2016 00:38

antrik reviewed Nov 7, 2016

View reviewed changes

dlrobertson changed the title ~~[WIP] Use Uuid instead of i64 for handler keys~~ Use autoincrementing number for handler ids Nov 7, 2016

dlrobertson force-pushed the use-uuid branch from b8fdf19 to 2deca67 Compare November 8, 2016 01:19

dlrobertson commented Nov 8, 2016

View reviewed changes

dlrobertson mentioned this pull request Nov 8, 2016

macos: Make sure to decrement the ref count of the mach port on ChannelClosed #116

Open

dlrobertson force-pushed the use-uuid branch from 2deca67 to 2e1081c Compare November 10, 2016 02:27

bors-servo merged commit 2e1081c into servo:master Nov 10, 2016

dlrobertson deleted the use-uuid branch November 11, 2016 01:22

dlrobertson mentioned this pull request Nov 11, 2016

Linux OsIpcReceiverSet - Switch to use mio #94

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use autoincrementing number for handler ids #105

Use autoincrementing number for handler ids #105

dlrobertson commented Oct 1, 2016 •

edited

Loading

vvuk commented Oct 14, 2016

dlrobertson commented Oct 17, 2016

vvuk commented Oct 18, 2016

dlrobertson commented Oct 18, 2016 •

edited

Loading

antrik commented Oct 19, 2016

dlrobertson commented Oct 25, 2016

bors-servo commented Oct 25, 2016

dlrobertson commented Nov 7, 2016

antrik left a comment

antrik Nov 7, 2016

dlrobertson commented Nov 8, 2016

dlrobertson Nov 8, 2016

KiChjang Nov 8, 2016

dlrobertson Nov 8, 2016

antrik Nov 8, 2016

dlrobertson Nov 10, 2016

antrik Nov 10, 2016

antrik commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

dlrobertson commented Nov 11, 2016

emilio commented Nov 11, 2016

Use autoincrementing number for handler ids #105

Use autoincrementing number for handler ids #105

Conversation

dlrobertson commented Oct 1, 2016 • edited Loading

vvuk commented Oct 14, 2016

dlrobertson commented Oct 17, 2016

vvuk commented Oct 18, 2016

dlrobertson commented Oct 18, 2016 • edited Loading

antrik commented Oct 19, 2016

dlrobertson commented Oct 25, 2016

bors-servo commented Oct 25, 2016

dlrobertson commented Nov 7, 2016

antrik left a comment

Choose a reason for hiding this comment

antrik Nov 7, 2016

Choose a reason for hiding this comment

dlrobertson commented Nov 8, 2016

dlrobertson Nov 8, 2016

Choose a reason for hiding this comment

KiChjang Nov 8, 2016

Choose a reason for hiding this comment

dlrobertson Nov 8, 2016

Choose a reason for hiding this comment

antrik Nov 8, 2016

Choose a reason for hiding this comment

dlrobertson Nov 10, 2016

Choose a reason for hiding this comment

antrik Nov 10, 2016

Choose a reason for hiding this comment

antrik commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

dlrobertson commented Nov 11, 2016

emilio commented Nov 11, 2016

dlrobertson commented Oct 1, 2016 •

edited

Loading

dlrobertson commented Oct 18, 2016 •

edited

Loading