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

piscisaureus · 2019-11-13T22:51:14Z

No description provided.

piscisaureus · 2019-11-13T23:02:09Z

@dtacalau Can you check this PR for the memory leak you discovered?

dtacalau · 2019-11-14T08:19:22Z

@dtacalau Can you check this PR for the memory leak you discovered?

Sure, will check it!

dtacalau · 2019-11-14T10:35:41Z

@piscisaureus I've checked this PR on all tests, here are the results:

For test from issue Windows mio leaks memory when poll does not retrieve any event #1146, it still reproducing:

~~Dr.M~~ Error #5: LEAK 128 direct bytes 0x0000008d40fffb00-0x0000008d40fffb80 + 40 indirect bytes
~~Dr.M~~ # 0 replace_RtlAllocateHeap                                                    [d:\drmemory_package\common\alloc_replace.c:3771]
~~Dr.M~~ # 1 alloc::alloc::alloc                                                        [/rustc/625451e376bb2e5283fc4741caa0a3e8a2ca4d54\src\liballoc\alloc.rs:84]
~~Dr.M~~ # 2 alloc::alloc::exchange_malloc                                              [/rustc/625451e376bb2e5283fc4741caa0a3e8a2ca4d54\src\liballoc\alloc.rs:206]
~~Dr.M~~ # 3 alloc::sync::Arc<>::new<>                                                  [/rustc/625451e376bb2e5283fc4741caa0a3e8a2ca4d54\src\liballoc\sync.rs:306]
~~Dr.M~~ # 4 alloc::sync::Arc<>::pin<>                                                  [/rustc/625451e376bb2e5283fc4741caa0a3e8a2ca4d54\src\liballoc\sync.rs:318]
~~Dr.M~~ # 5 mio::sys::windows::selector::SelectorInner::_alloc_sock_for_rawsocket      [C:\work\git\mio\src\sys\windows\selector.rs:608]
~~Dr.M~~ # 6 mio::sys::windows::selector::SelectorInner::register<>                     [C:\work\git\mio\src\sys\windows\selector.rs:466]
~~Dr.M~~ # 7 mio::sys::windows::selector::Selector::register<>                          [C:\work\git\mio\src\sys\windows\selector.rs:333]
~~Dr.M~~ # 8 mio::sys::windows::tcp::{{impl}}::register                                 [C:\work\git\mio\src\sys\windows\tcp.rs:453]
~~Dr.M~~ # 9 mio::net::tcp::listener::{{impl}}::register                                [C:\work\git\mio\src\net\tcp\listener.rs:142]
~~Dr.M~~ #10 core::sync::atomic::AtomicUsize::load                                      [/rustc/625451e376bb2e5283fc4741caa0a3e8a2ca4d54\src\libcore\sync\atomic.rs:1296]
~~Dr.M~~ #11 mio::poll::Registry::register<>                                            [C:\work\git\mio\src\poll.rs:494]
~~Dr.M~~
~~Dr.M~~ ERRORS FOUND:
~~Dr.M~~       0 unique,     0 total unaddressable access(es)
~~Dr.M~~       4 unique,     4 total uninitialized access(es)
~~Dr.M~~       0 unique,     0 total invalid heap argument(s)
~~Dr.M~~       0 unique,     0 total GDI usage error(s)
~~Dr.M~~       0 unique,     0 total handle leak(s)
~~Dr.M~~       0 unique,     0 total warning(s)
~~Dr.M~~       1 unique,     1 total,    168 byte(s) of leak(s)
~~Dr.M~~       0 unique,     0 total,      0 byte(s) of possible leak(s)
~

Tests from registering.rs also leak, Multiple mio tests leak memory on Windows #1150

running 5 tests
test udp_register_multiple_event_loops ... ok
test tcp_register_multiple_event_loops ... ok
test registering_after_deregistering ... ok
test register_deregister ... ok
test reregister_different_without_poll ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
~~Dr.M~~ Error #5: LEAK 128 direct bytes 0x032c14e0-0x032c1560 + 0 indirect bytes
~~Dr.M~~ Error #6: POSSIBLE LEAK 200 direct bytes 0x032c9820-0x032c98e8 + 26 indirect bytes
~~Dr.M~~ Error #7: POSSIBLE LEAK 60 direct bytes 0x032c9908-0x032c9944 + 0 indirect bytes
~~Dr.M~~ ERRORS FOUND:
~~Dr.M~~       1 unique,     1 total,    128 byte(s) of leak(s)
~~Dr.M~~       2 unique,     2 total,    286 byte(s) of possible leak(s)

dtacalau · 2019-11-14T10:37:29Z

src/sys/windows/selector.rs

            self.pending_evts = self.user_evts;
+            forget(self_arc.clone());


This looks strange, why call forget on the clone and not on the self_arc directly?

self_arc is a reference here. Dropping a reference does is no-op, therefore ’forget’ing a reference instead of dropping doesn't make any difference.

dtacalau · 2019-11-14T11:41:14Z

src/sys/windows/selector.rs

@@ -626,7 +585,7 @@ impl SelectorInner {
                n += 1;
                continue;
            }
-            let sock_arc = Arc::from_raw(iocp_event.overlapped() as *const Mutex<SockState>);
+            let sock_arc: Pin<Arc<Mutex<SockState>>> = transmute_copy(&iocp_event.overlapped());


is this a perfect copy without increasing the ref count?

Yep. It's basically reinterpret_cast + copy.

dtacalau · 2019-11-14T17:42:58Z

0001-Added-support-to-cancel-sock-state-on-selector-drop.zip

@piscisaureus here's a patch done on your branch which add support for cancel on drop. Using this patch there are no more leaks. Let me know what you think!

…nt memory leaks for sockets which have been registered and haven't been deregistered or sockets with no events polled. Signed-off-by: Daniel Tacalau <dst4096@gmail.com>

piscisaureus · 2019-11-15T01:04:31Z

@dtacalau

I've addressed the correctness issues I found in your patch (it's still messy nonetheless) and added a counter to check for leaked SockState (now SockStateInner) objects. Based on the counter I'd say that there are no SockStateInner objects being leaked. However DrMemory still finds leaks for me, so I'm pretty sure it's something else that's leaking.

piscisaureus · 2019-11-15T01:06:39Z

@dtacalau I would actually suggest to land b994c29 first. It may not fix the leak but it does clean up a bunch of other stuff.

dtacalau · 2019-11-15T10:39:13Z

@dtacalau I would actually suggest to land b994c29 first. It may not fix the leak but it does clean up a bunch of other stuff.

@piscisaureus not sure about merging b994c29. It looks like a lot of refactoring done and release date is getting close, test coverage could be better, feels risky. I'd defer this decision to @carllerche and @Thomasdezeeuw.

But if we still want to merge this, I'd suggest splitting it in multiple, more trackable, smaller commits, i.e: one comit for fix windows Sock ref count, multiple misc clean-ups comits.

dtacalau · 2019-11-15T10:44:42Z

@piscisaureus Have a look at #1147, I've redone my initial mem leak fix by just adding the cancel sockstate on drop on current master and all the leaks are gone. Not sure if the correctness issues you've mentioned are gone, can you take a look? We could merge this to keep things simple.

piscisaureus · 2019-11-16T01:22:45Z

@dtacalau

@piscisaureus not sure about merging b994c29. It looks like a lot of refactoring done and release date is getting close, test coverage could be better, feels risky. I'd defer this decision to @carllerche and @Thomasdezeeuw.

But if we still want to merge this, I'd suggest splitting it in multiple, more trackable, smaller commits, i.e: one comit for fix windows Sock ref count, multiple misc clean-ups comits.

Fair enough. WDYT about #1154? That at least gets rid of an unnecessary Box around Overlapped and that wonky self_overlapped thing.

Thomasdezeeuw · 2019-11-16T10:37:58Z

Is the plan to merge #1154 first? Because I think there is some overlap/conflicts with this pr.

dtacalau · 2019-11-17T16:30:48Z

Fair enough. WDYT about #1154? That at least gets rid of an unnecessary Box around Overlapped and that wonky self_overlapped thing.

@piscisaureus #1154 looks good! We still need to fix the mem leak afterwards.

dtacalau · 2019-11-17T16:38:44Z

Is the plan to merge #1154 first? Because I think there is some overlap/conflicts with this pr.

@Thomasdezeeuw Here's how I see the plan: I also think #1154 should be merged first. The misc clean-ups done by @piscisaureus are definitely useful, but, to me, it seemed too many changes at once, so I suggested splitting them in multiple commits.

At the same time we should come up with a fix for the memory leaks (delete remaining sock states when doing selector drop).

Thomasdezeeuw · 2019-11-30T10:49:53Z

#1154 is merged, what is left to do here?

dtacalau · 2019-11-30T15:16:10Z

#1154 is merged, what is left to do here?

@Thomasdezeeuw there are several misc clean-ups remaining. @piscisaureus do you want to create a separate commit with remaining clean-ups so we can merge them?

piscisaureus · 2019-11-30T17:37:11Z

@Thomasdezeeuw there are several misc clean-ups remaining. @piscisaureus do you want to create a separate commit with remaining clean-ups so we can merge them?

I'll do it when I have time, but I won't have time for at least a week and a half.

These misc cleanups are not super important IMO, so just forget about them for now. There were a few small bug fixes contained in the patch. When I have the time I'll prioritize on landing those.

piscisaureus force-pushed the win branch 2 times, most recently from d6819d9 to 8b13d05 Compare November 13, 2019 23:00

piscisaureus force-pushed the win branch 4 times, most recently from 6ab4edf to d6e7e79 Compare November 14, 2019 01:11

Fix windows SockState memory leak

8715494

piscisaureus force-pushed the win branch from d6e7e79 to 8265ca6 Compare November 14, 2019 01:17

piscisaureus changed the title ~~Fix IO_STATUS_BLOCK/AFD_POLL_INFO reference counting~~ Fix windows SockState reference counting + misc clean-ups Nov 14, 2019

dtacalau reviewed Nov 14, 2019

View reviewed changes

dtacalau mentioned this pull request Nov 14, 2019

Multiple mio tests leak memory on Windows #1150

Closed

piscisaureus and others added 2 commits November 14, 2019 10:51

Miscellaneous windows implementation clean-ups

eade31e

Added support to cancel sock state on selector drop. This would preve…

03514d0

…nt memory leaks for sockets which have been registered and haven't been deregistered or sockets with no events polled. Signed-off-by: Daniel Tacalau <dst4096@gmail.com>

piscisaureus force-pushed the win branch from 8265ca6 to 03514d0 Compare November 14, 2019 18:51

WIP

77872be

piscisaureus force-pushed the win branch 2 times, most recently from 77872be to 6003655 Compare November 15, 2019 01:00

piscisaureus force-pushed the win branch from 6003655 to 77872be Compare November 16, 2019 01:15

piscisaureus mentioned this pull request Nov 16, 2019

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

Closed

piscisaureus closed this Nov 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

piscisaureus commented Nov 13, 2019

piscisaureus commented Nov 13, 2019

dtacalau commented Nov 14, 2019

dtacalau commented Nov 14, 2019 •

edited

Loading

dtacalau Nov 14, 2019

piscisaureus Nov 14, 2019 •

edited

Loading

dtacalau Nov 14, 2019

piscisaureus Nov 14, 2019

dtacalau commented Nov 14, 2019

piscisaureus commented Nov 15, 2019

piscisaureus commented Nov 15, 2019

dtacalau commented Nov 15, 2019

dtacalau commented Nov 15, 2019 •

edited

Loading

piscisaureus commented Nov 16, 2019 •

edited

Loading

Thomasdezeeuw commented Nov 16, 2019

dtacalau commented Nov 17, 2019

dtacalau commented Nov 17, 2019

Thomasdezeeuw commented Nov 30, 2019

dtacalau commented Nov 30, 2019

piscisaureus commented Nov 30, 2019

		self.pending_evts = self.user_evts;
		forget(self_arc.clone());

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

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

Conversation

piscisaureus commented Nov 13, 2019

piscisaureus commented Nov 13, 2019

dtacalau commented Nov 14, 2019

dtacalau commented Nov 14, 2019 • edited Loading

dtacalau Nov 14, 2019

Choose a reason for hiding this comment

piscisaureus Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

dtacalau Nov 14, 2019

Choose a reason for hiding this comment

piscisaureus Nov 14, 2019

Choose a reason for hiding this comment

dtacalau commented Nov 14, 2019

piscisaureus commented Nov 15, 2019

piscisaureus commented Nov 15, 2019

dtacalau commented Nov 15, 2019

dtacalau commented Nov 15, 2019 • edited Loading

piscisaureus commented Nov 16, 2019 • edited Loading

Thomasdezeeuw commented Nov 16, 2019

dtacalau commented Nov 17, 2019

dtacalau commented Nov 17, 2019

Thomasdezeeuw commented Nov 30, 2019

dtacalau commented Nov 30, 2019

piscisaureus commented Nov 30, 2019

dtacalau commented Nov 14, 2019 •

edited

Loading

piscisaureus Nov 14, 2019 •

edited

Loading

dtacalau commented Nov 15, 2019 •

edited

Loading

piscisaureus commented Nov 16, 2019 •

edited

Loading