Simplify windows SockState reference counting #1154

piscisaureus · 2019-11-16T01:19:20Z

No description provided.

Thomasdezeeuw

I'm not too sure about the use of transmute_copy and some small nits, otherwise this LGTM.

src/sys/windows/mod.rs

src/sys/windows/selector.rs

dtacalau

Looks good! Nothing else to add apart from what Thomas said.

carllerche · 2019-11-19T17:57:46Z

I believe that Arc implies Pin (since you cannot get a &mut), so having both would be redundant?

Thomasdezeeuw · 2019-11-19T18:47:29Z

I believe that Arc implies Pin (since you cannot get a &mut), so having both would be redundant?

This not correct. Pin implies that the object it points to (P) must be pinned in memory and thus may not be moved, unless it implements Unpin. Since the memory is used by the kernel it must not be moved or the kernel might write into memory we don't own (any more). So Pin<Arc<T>> is completely valid.

Come to thing of it, we might want to add PhantomPinned to the memory that may not be moved to ensure this.

piscisaureus · 2019-11-19T19:10:56Z

Come to thing of it, we might want to add PhantomPinned to the memory that may not be moved to ensure this.

It is already there: https://github.com/tokio-rs/mio/pull/1154/files#diff-8e89b097d3e3119d177930239584d29eR103

carllerche · 2019-11-19T19:32:43Z

@Thomasdezeeuw you can't move memory out of an Arc w/o unsafe anyway? Or am i missing something?

dtacalau · 2019-11-20T15:16:38Z

@carllerche , @Thomasdezeeuw Not sure where this PR will end up, if it's going to get merged or not, I'm trying to get a resolution with the mem leak fix . I've also pinged @piscisaureus . Could you take a look at what I'm proposing here
#1147 (comment) and let me know what you think, yay or nay?

Thomasdezeeuw · 2019-11-22T10:42:22Z

@Thomasdezeeuw you can't move memory out of an Arc w/o unsafe anyway? Or am i missing something?

You're right, but it more about showing the intent of the code through the type system. There is for example: Arc::get_mut, which would allow you to move the memory, Pin would disallow this.

Thomasdezeeuw · 2019-11-22T11:07:48Z

@piscisaureus I've created piscisaureus#1, if that is OK & merged then this can be merged.

dtacalau · 2019-11-23T20:59:42Z

@Thomasdezeeuw, @piscisaureus I've created piscisaureus#2, I think I finally nailed the leak fix as simple as possible :). With this leak fix, dr memory does not reports any leaks in the tests.

Thomasdezeeuw · 2019-11-23T22:41:51Z

@dtacalau If I understand the problem correctly I don't think your fix will work when a socket has been registered (i.e. the ref count increased), but has no events for it when the Selector is being dropped.

Also I'm not a fan of allocating Events in the Drop impl.

dtacalau · 2019-11-24T10:15:25Z

@dtacalau If I understand the problem correctly I don't think your fix will work when a socket has been registered (i.e. the ref count increased), but has no events for it when the Selector is being dropped.

@Thomasdezeeuw it works, do you mean this kind of test? Because this was the test I used to discover the leak initially. Also it's been my main method of testing the leak fix works. Let me know if you have other test in mind for which you think the leak fix does not work.

#[test]
fn no_leak_registered_listener_no_events()
{
    let (mut poll, mut events) = init_with_poll();

    let listener = TcpListener::bind(any_local_address()).unwrap();
    let address = listener.local_addr().unwrap();

    poll.registry()
        .register(&listener, ID1, Interests::READABLE)
        .expect("unable to register TCP listener");

      
    poll.poll(&mut events, Some(std::time::Duration::from_millis(500))).expect("unable to poll");
}

Also I'm not a fan of allocating Events in the Drop impl.

@Thomasdezeeuw what's wrong with that?

Thomasdezeeuw · 2019-11-24T15:48:27Z

Also I'm not a fan of allocating Events in the Drop impl.

@Thomasdezeeuw what's wrong with that?

Allocating memory using it once and then dropping it again, especially in the Drop impl, is an unneeded waste. We can use an stack allocated array of raw events and the raw system call and do minimal amount of processing.

Also for the test: I'm a little surprised that it leaks memory. Because I wouldn't think that draining the events would resolve it, as I wouldn't expect any events for the TcpListener.

dtacalau · 2019-11-24T16:45:44Z

Allocating memory using it once and then dropping it again, especially in the Drop impl, is an unneeded waste. We can use an stack allocated array of raw events and the raw system call and do minimal amount of processing.

@Thomasdezeeuw I can do it optimal if that's the issue.

Also for the test: I'm a little surprised that it leaks memory. Because I wouldn't think that draining the events would resolve it, as I wouldn't expect any events for the TcpListener.

It works because draining the events will also release the SockState Arc reference which is blocked during afd poll call.

dtacalau · 2019-11-25T17:28:53Z

Allocating memory using it once and then dropping it again, especially in the Drop impl, is an
unneeded waste. We can use an stack allocated array of raw events and the raw system call and do > minimal amount of processing.

@Thomasdezeeuw simplified, please have a look piscisaureus@92e037e

A reference of the sock state Arc is blocked by a pending AFD poll request, this reference will leak if there's no explicit poll done before the program terminates. That's why doing an explicit "select" during inner selector drop ensures all sock state references are freed. Fixes: tokio-rs#1146 Signed-off-by: Daniel Tacalau <dst4096@gmail.com>

dtacalau · 2019-11-26T09:23:30Z

@Thomasdezeeuw , @carllerche please review so we can merge this ASAP.

Thomasdezeeuw · 2019-11-26T10:46:19Z

I still don't entirely agree with the last commit, but I don't have enough time to properly review it. So let's move that to another pr so we can this merged.

dtacalau · 2019-11-26T11:54:06Z

src/sys/windows/selector.rs

+/// To revert see `from_overlapped`.
+fn into_overlapped(sock_state: Pin<Arc<Mutex<SockState>>>) -> PVOID {
+    let overlapped_ptr: *const Mutex<SockState> =
+        unsafe { Arc::into_raw(Pin::into_inner_unchecked(sock_state)) };


This breaks CI tests on Windows. If this gets merged as it is, it will block testing on Windows side because CI will always fail here.

error[E0658]: use of unstable library feature 'pin_into_inner'
--> src\sys\windows\selector.rs:279:32
|
279 | unsafe { Arc::into_raw(Pin::into_inner_unchecked(sock_state)) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: for more information, see rust-lang/rust#60245

error: aborting due to previous error

For more information about this error, try rustc --explain E0658.
error: Could not compile mio.

To learn more, run the command again with --verbose.

dtacalau · 2019-11-26T12:02:00Z

I still don't entirely agree with the last commit, but I don't have enough time to properly review it. So let's move that to another pr so we can this merged.

@Thomasdezeeuw I would need the leak fix commit merged, so if you don't have time to review it now, I'd suggest merging it as it is and review it later. It fixed the leaks, and that's good enough for now IMO.

dtacalau · 2019-11-27T10:09:48Z

@Thomasdezeeuw what's the plan with this PR? When do you intend to merge it? The CI error seems blocking 279 | unsafe { Arc::into_raw(Pin::into_inner_unchecked(sock_state)) }; , at the same time the leak fix depends on your comit from this PR.

Thomasdezeeuw · 2019-11-27T11:13:56Z

I've created #1172 to fix the CI. @piscisaureus can you drop the last commit, then we can merge this. @dtacalau can you crate a new pr with the last commit? So we can discuss it further without stalling this pr.

Going forward I would prefer to focus the prs on single issues, prs like these take way too long to get merged even without all the additional commits added on. In the future let's focus on a single (smaller) change in prs and try to get them merged faster. Note that I'm also far from perfect in this regard with my own prs.

dtacalau · 2019-11-27T12:07:55Z

I've created #1172 to fix the CI. @piscisaureus can you drop the last commit, then we can merge >this. @dtacalau can you crate a new pr with the last commit? So we can discuss it further without >stalling this pr.

@Thomasdezeeuw ok, new PR it is. Would appreciate it if you'd review it sooner rather than later.

Going forward I would prefer to focus the prs on single issues, prs like these take way too long to >get merged even without all the additional commits added on. In the future let's focus on a single
(smaller) change in prs and try to get them merged faster. Note that I'm also far from perfect in >this regard with my own prs.

I agree 100% with what you've said. Also, on another note, seems to me we're spending too much time trying to get the code perfect. Doesn't make much sense atm, because, at least on Windows, the code base is still young, things gets changed often. I think we'll be more efficient with our time doing good enough changes, and leave the perfect for later when the code base is stable.

carllerche · 2019-11-27T18:16:04Z

@Thomasdezeeuw I merged master, which should fix CI. I think we should move forward w/ this now and iterate in follow up PRs. wdyt?

carllerche

I'm fine w/ this PR as it is an improvement. IMO we can continue iterating on master and hold off release of 0.7 until we are good w/ all the details.

Thomasdezeeuw · 2019-11-28T14:22:23Z

I've merged this to get things moving, but I've reopened #1146 to continue there.

A reference of the sock state Arc is blocked by a pending AFD poll request, this reference will leak if there's no explicit poll done before the program terminates. That's why doing an explicit "select" during inner selector drop ensures all sock state references are freed. Fixes: tokio-rs#1146 Signed-off-by: Daniel Tacalau <dst4096@gmail.com>

piscisaureus mentioned this pull request Nov 16, 2019

Fix windows SockState reference counting + misc clean-ups #1149

Closed

Thomasdezeeuw requested changes Nov 16, 2019

View reviewed changes

src/sys/windows/mod.rs Show resolved Hide resolved

src/sys/windows/selector.rs Outdated Show resolved Hide resolved

src/sys/windows/selector.rs Outdated Show resolved Hide resolved

src/sys/windows/selector.rs Outdated Show resolved Hide resolved

dtacalau reviewed Nov 17, 2019

View reviewed changes

piscisaureus added a commit to piscisaureus/mio that referenced this pull request Nov 18, 2019

Simplify windows SockState reference counting (tokio-rs#1154)

2c29930

piscisaureus force-pushed the simplify branch from 63df501 to 2c29930 Compare November 18, 2019 01:00

piscisaureus requested a review from Thomasdezeeuw November 18, 2019 01:00

dtacalau mentioned this pull request Nov 19, 2019

fix issue #1146 mio leaks memory when poll does not retrieve any event #1147

Closed

piscisaureus and others added 3 commits November 25, 2019 19:44

Simplify windows SockState reference counting (tokio-rs#1154)

88407fb

Remove use of transmute_copy (tokio-rs#1154)

5ef0330

piscisaureus force-pushed the simplify branch from 5968b26 to 3d654f7 Compare November 26, 2019 03:48

dtacalau approved these changes Nov 26, 2019

View reviewed changes

dtacalau suggested changes Nov 26, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into simplify

b795c98

carllerche approved these changes Nov 27, 2019

View reviewed changes

dtacalau pushed a commit to dtacalau/mio that referenced this pull request Nov 27, 2019

Simplify windows SockState reference counting (tokio-rs#1154)

da34692

Thomasdezeeuw approved these changes Nov 28, 2019

View reviewed changes

Thomasdezeeuw merged commit 2b68a4b into tokio-rs:master Nov 28, 2019

Thomasdezeeuw pushed a commit that referenced this pull request Nov 28, 2019

Simplify windows SockState reference counting (#1154)

6707055

Thomasdezeeuw added a commit that referenced this pull request Nov 28, 2019

Remove use of transmute_copy (#1154)

5554b23

Thomasdezeeuw mentioned this pull request Nov 28, 2019

More changes for the leak fix, issue 1146 #1168

Merged

dtacalau mentioned this pull request Nov 29, 2019

Multiple mio tests leak memory on Windows #1150

Closed

dtacalau pushed a commit to dtacalau/mio that referenced this pull request Dec 4, 2019

Simplify windows SockState reference counting (tokio-rs#1154)

417f8ca

dtacalau pushed a commit to dtacalau/mio that referenced this pull request Dec 4, 2019

Remove use of transmute_copy (tokio-rs#1154)

bd7e874

dtacalau mentioned this pull request Dec 5, 2019

Remove some mutexes from Windows implementation #1182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify windows SockState reference counting #1154

Simplify windows SockState reference counting #1154

piscisaureus commented Nov 16, 2019

Thomasdezeeuw left a comment

dtacalau left a comment

carllerche commented Nov 19, 2019

Thomasdezeeuw commented Nov 19, 2019

piscisaureus commented Nov 19, 2019

carllerche commented Nov 19, 2019

dtacalau commented Nov 20, 2019

Thomasdezeeuw commented Nov 22, 2019

Thomasdezeeuw commented Nov 22, 2019

dtacalau commented Nov 23, 2019 •

edited

Loading

Thomasdezeeuw commented Nov 23, 2019

dtacalau commented Nov 24, 2019 •

edited

Loading

Thomasdezeeuw commented Nov 24, 2019

dtacalau commented Nov 24, 2019

dtacalau commented Nov 25, 2019

dtacalau commented Nov 26, 2019

Thomasdezeeuw commented Nov 26, 2019

dtacalau Nov 26, 2019

dtacalau commented Nov 26, 2019

dtacalau commented Nov 27, 2019 •

edited

Loading

Thomasdezeeuw commented Nov 27, 2019

dtacalau commented Nov 27, 2019 •

edited

Loading

carllerche commented Nov 27, 2019

carllerche left a comment

Thomasdezeeuw commented Nov 28, 2019

Simplify windows SockState reference counting #1154

Simplify windows SockState reference counting #1154

Conversation

piscisaureus commented Nov 16, 2019

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

dtacalau left a comment

Choose a reason for hiding this comment

carllerche commented Nov 19, 2019

Thomasdezeeuw commented Nov 19, 2019

piscisaureus commented Nov 19, 2019

carllerche commented Nov 19, 2019

dtacalau commented Nov 20, 2019

Thomasdezeeuw commented Nov 22, 2019

Thomasdezeeuw commented Nov 22, 2019

dtacalau commented Nov 23, 2019 • edited Loading

Thomasdezeeuw commented Nov 23, 2019

dtacalau commented Nov 24, 2019 • edited Loading

Thomasdezeeuw commented Nov 24, 2019

dtacalau commented Nov 24, 2019

dtacalau commented Nov 25, 2019

dtacalau commented Nov 26, 2019

Thomasdezeeuw commented Nov 26, 2019

dtacalau Nov 26, 2019

Choose a reason for hiding this comment

dtacalau commented Nov 26, 2019

dtacalau commented Nov 27, 2019 • edited Loading

Thomasdezeeuw commented Nov 27, 2019

dtacalau commented Nov 27, 2019 • edited Loading

carllerche commented Nov 27, 2019

carllerche left a comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Nov 28, 2019

dtacalau commented Nov 23, 2019 •

edited

Loading

dtacalau commented Nov 24, 2019 •

edited

Loading

dtacalau commented Nov 27, 2019 •

edited

Loading

dtacalau commented Nov 27, 2019 •

edited

Loading