Remove net2 dependency #1029

Thomasdezeeuw · 2019-07-11T09:34:02Z

This initial comment only remove net2 from TcpStream::connect, on Unix platforms (expect for iOS, macOS and Solaris) this reduces the number of system calls from three to two.

@carllerche this does increase the complexity a bit, do we still want to continue down this route?

Closes #841.
Closes #1045.

src/sys/unix/tcp.rs

carllerche

Looks fine to me. One thought re copyright.

Thomasdezeeuw · 2019-07-11T21:14:29Z

Note to self: check for the same problem as in #1031 in the changes I'm going to make.

carllerche · 2019-07-15T22:18:48Z

We should be fine to move forward w/ this if you want to.

Thomasdezeeuw · 2019-07-16T10:01:12Z

@carllerche I was thinking about waiting until the Windows rewrite as I imagine there will a number of merge conflicts. But I'll finish this.

Thomasdezeeuw · 2019-07-21T14:42:05Z

I've force pushed a commit. It is rebased on master and contains the code for Windows as well, still limited to TcpStream::connect.

Thomasdezeeuw · 2019-07-21T14:45:51Z

src/sys/windows/tcp.rs

+        })
+        .and_then(|socket| {
+            // Required for a future `connect_overlapped` operation to be
+            // executed successfully.


After the rewrite does this still apply?

Thomasdezeeuw · 2019-07-21T15:20:41Z

After some trial and error this now works on Windows. Can some double check the code since I just developed it of the documentation and didn't run anything locally.

Thomasdezeeuw · 2019-07-21T18:03:16Z

I'm getting the Either the application has not called WSAStartup, or WSAStartup failed. error on Windows, can someone with some Windows knowledge help me with that?

carllerche · 2019-07-21T18:07:15Z

cc @piscisaureus @PerfectLaugh ^^

We probably are missing a run_once call to WSAStartup?

PerfectLaugh · 2019-07-21T18:12:06Z

The WSAStartup should be already executed by std library. Of course it would be exception of you don't import the std::net

…

On Mon, Jul 22, 2019, 2:07 AM Carl Lerche ***@***.***> wrote: cc @piscisaureus <https://github.com/piscisaureus> @PerfectLaugh <https://github.com/PerfectLaugh> ^^ We probably are missing a run_once call to WSAStartup? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1029?email_source=notifications&email_token=AE43NARQCUGMLNU25B76RRLQASQVRA5CNFSM4IA422TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2OITWA#issuecomment-513575384>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE43NAS7HJFDNPRUK2X64GLQASQVRANCNFSM4IA422TA> .

PerfectLaugh · 2019-07-21T18:12:50Z

https://github.com/rust-lang/rust/blob/00859e3e653973120006aaf3227823062dde1ba7/src/libstd/sys/windows/net.rs#L31

…

On Mon, Jul 22, 2019, 2:11 AM PerfectLaugh ***@***.***> wrote: The WSAStartup should be already executed by std library. Of course it would be exception of you don't import the std::net On Mon, Jul 22, 2019, 2:07 AM Carl Lerche ***@***.***> wrote: > cc @piscisaureus <https://github.com/piscisaureus> @PerfectLaugh > <https://github.com/PerfectLaugh> ^^ > > We probably are missing a run_once call to WSAStartup? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1029?email_source=notifications&email_token=AE43NARQCUGMLNU25B76RRLQASQVRA5CNFSM4IA422TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2OITWA#issuecomment-513575384>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AE43NAS7HJFDNPRUK2X64GLQASQVRANCNFSM4IA422TA> > . >

carllerche · 2019-07-21T18:33:11Z

@PerfectLaugh How can we ensure it gets called?

carllerche · 2019-07-21T18:34:01Z

@Thomasdezeeuw In case you didn't know, you can run cargo check --target x86_64-pc-windows-msvc while not on windows to at least make sure things will compile.

PerfectLaugh · 2019-07-21T18:34:42Z

What is the test that throw error btw?

…

On Mon, Jul 22, 2019, 2:33 AM Carl Lerche ***@***.***> wrote: @PerfectLaugh <https://github.com/PerfectLaugh> How can we ensure it gets called? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1029?email_source=notifications&email_token=AE43NAUUT462I2FOMLC74K3QASTWZA5CNFSM4IA422TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2OJA7I#issuecomment-513577085>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE43NAXJWEI2ESBHMRCCQILQASTWZANCNFSM4IA422TA> .

Thomasdezeeuw · 2019-07-21T18:37:12Z

@Thomasdezeeuw In case you didn't know, you can run cargo check --target x86_64-pc-windows-msvc while not on windows to at least make sure things will compile.

I know, but I used build which doesn't check tests (I just found out) :).

carllerche · 2019-07-21T18:37:18Z

@PerfectLaugh it looks like init() is only called when using a TCP or UDP type, so it is possible for us to hit cases where we need WSAStartup() called.

Thomasdezeeuw · 2019-07-21T18:37:51Z

Seems that we need basically a function in std::net for init to be called.

Thomasdezeeuw · 2019-07-21T18:39:57Z

Oh TcpStream and TcpListener don't implement to/as/from_raw_socket, want me to add that? Since I've already used it in a test.

PerfectLaugh · 2019-07-21T18:43:30Z

I think it is good to add that but notice the comment I added on #1041 #1041 (comment)

…

On Mon, Jul 22, 2019, 2:40 AM Thomas de Zeeuw ***@***.***> wrote: Oh TcpStream and TcpListener don't implement to/as/from_raw_socket, want me to add that? Since I've already used it in a test. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1029?email_source=notifications&email_token=AE43NASLFK4ZXEN6DSPNQG3QASUQFA5CNFSM4IA422TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2OJEMA#issuecomment-513577520>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE43NATHYFPONPO3NSK63B3QASUQFANCNFSM4IA422TA> .

PerfectLaugh · 2019-07-21T18:47:32Z

Maybe we can do WSAStartup by ourselves? It is not bad to multiple call this function to make sure the poll can work without tcp/udp

…

On Mon, Jul 22, 2019, 2:43 AM PerfectLaugh ***@***.***> wrote: I think it is good to add that but notice the comment I added on #1041 #1041 (comment) On Mon, Jul 22, 2019, 2:40 AM Thomas de Zeeuw ***@***.***> wrote: > Oh TcpStream and TcpListener don't implement to/as/from_raw_socket, want > me to add that? Since I've already used it in a test. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1029?email_source=notifications&email_token=AE43NASLFK4ZXEN6DSPNQG3QASUQFA5CNFSM4IA422TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2OJEMA#issuecomment-513577520>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AE43NATHYFPONPO3NSK63B3QASUQFANCNFSM4IA422TA> > . >

Thomasdezeeuw · 2019-07-21T18:58:04Z

I've disabled connection_reset_by_peer for now as it requires net2.

carllerche · 2019-07-21T22:53:52Z

@Thomasdezeeuw it seems ok to maintain a dev-dependency on net2.

Thomasdezeeuw · 2019-07-22T10:11:45Z

I've added a dummy listener to test_close_on_drop as a workaround and opened #1046.

Same strategy as for the TCP types; using SOCK_NONBLOCK and SOCK_CLOEXEC where possible.

This ensures WSAStartup is called on Windows and initialised env_logger.

I forgot it...

Thomasdezeeuw · 2019-07-22T11:53:40Z

So I've an init function to the tests that will create a TcpListener (from standard library) to trigger it to call WSAStartup, so the tests run fine now. However now the same problem occurs in the examples. I think we need to resolve #1046 before this can be merged.

The standard library calls WSAStartup for us, ensuring (with an assert) that the return value is ok. This means that we can't initialise it ourselves and need to let the standard library do it for us. To work around this we create and drop a UdpSocket (from std::net).

This is now handled inside the crate.

Thomasdezeeuw · 2019-08-03T17:03:14Z

I've added an init function to Windows' TCP types, matching what libstd does. I think this pr is ready, I haven't gotten around to UdpSocket but I want to get this merged to prevent further delays (and merge conflicts).

Thomasdezeeuw · 2019-08-06T19:17:05Z

@PerfectLaugh I can't request your review, but can you look at the Windows implementation?

PerfectLaugh · 2019-08-06T22:28:15Z

Looks good to me and have no issue of test in my computer.
Btw. You should look my pull request and try to make changes into your commit. #1042 so I can close my pull request then :)

Thomasdezeeuw · 2019-08-13T10:08:08Z

@asomers, @carllerche any chance you have some time to review this?

carllerche

👍

carllerche reviewed Jul 11, 2019

View reviewed changes

src/sys/unix/tcp.rs Outdated Show resolved Hide resolved

carllerche reviewed Jul 11, 2019

View reviewed changes

Thomasdezeeuw force-pushed the remove-net2 branch from 4c713dd to fffa694 Compare July 21, 2019 14:41

Thomasdezeeuw force-pushed the remove-net2 branch from fffa694 to 733dfa9 Compare July 21, 2019 14:45

Thomasdezeeuw commented Jul 21, 2019

View reviewed changes

Thomasdezeeuw mentioned this pull request Jul 21, 2019

Installing Rust takes 2 minutes longer on Windows then Linux on Azure #1044

Closed

Thomasdezeeuw force-pushed the remove-net2 branch from 66f37de to a39039e Compare July 21, 2019 17:44

Thomasdezeeuw mentioned this pull request Jul 21, 2019

Reduce TcpStream, TcpListener and UdpSocket API to the same as in libstd #1045

Closed

Thomasdezeeuw mentioned this pull request Jul 22, 2019

Determine how to do deal with WSAStartup on Windows #1046

Closed

Thomasdezeeuw added 11 commits July 22, 2019 13:40

Remove net2 dependency

b6ffd9b

Remove UdpSocket::from_socket

ea08139

Remove mention of net2 in documentation

b2541f0

Move creating of UdpSocket to sys module

5e44917

Renable connection_reset_by_peer on Unix

26f620b

Add a work around for test_close_on_drop on Windows

b26217b

Safe a systemcall in UdpSocket::bind on Unix

2a0f13f

Same strategy as for the TCP types; using SOCK_NONBLOCK and SOCK_CLOEXEC where possible.

Fix unused import warning on Windows

7ec0b81

Workaround WSAStartup problem in test_double_register

e37585b

Add init function to all function

8c33265

This ensures WSAStartup is called on Windows and initialised env_logger.

Add init to test_reregister_different_without_poll

dae8291

I forgot it...

Thomasdezeeuw force-pushed the remove-net2 branch from 2c1a3f2 to dae8291 Compare July 22, 2019 11:45

Thomasdezeeuw added 2 commits August 3, 2019 18:43

Drop network stack initialisation from tests

27a8f5c

This is now handled inside the crate.

Thomasdezeeuw marked this pull request as ready for review August 3, 2019 17:01

Thomasdezeeuw requested review from carllerche and asomers August 6, 2019 19:16

carllerche approved these changes Aug 13, 2019

View reviewed changes

carllerche merged commit f30debe into tokio-rs:master Aug 13, 2019

carllerche mentioned this pull request Aug 19, 2019

Remaining wepoll items. #1041

Closed

16 tasks

Thomasdezeeuw deleted the remove-net2 branch August 26, 2019 13:18

kleimkuhler mentioned this pull request Nov 6, 2019

Add from_std methods to TCP/Unix Stream/Listener #1124

Merged

kleimkuhler mentioned this pull request Dec 10, 2019

Introduce non-libstd socket builder methods #1211

Closed

Geobert mentioned this pull request Oct 9, 2020

[Question] Either the application has not called WSAStartup, or WSAStartup failed. (os error 10093) inejge/ldap3#56

Closed

Thomasdezeeuw mentioned this pull request Sep 19, 2023

Why cannot find WASStartup and WSACleanup in source code? rust-lang/socket2#472

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove net2 dependency #1029

Remove net2 dependency #1029

Thomasdezeeuw commented Jul 11, 2019 •

edited

carllerche left a comment

Thomasdezeeuw commented Jul 11, 2019

carllerche commented Jul 15, 2019

Thomasdezeeuw commented Jul 16, 2019

Thomasdezeeuw commented Jul 21, 2019

Thomasdezeeuw Jul 21, 2019

Thomasdezeeuw commented Jul 21, 2019

Thomasdezeeuw commented Jul 21, 2019

carllerche commented Jul 21, 2019

PerfectLaugh commented Jul 21, 2019 via email

PerfectLaugh commented Jul 21, 2019 via email

carllerche commented Jul 21, 2019

carllerche commented Jul 21, 2019

PerfectLaugh commented Jul 21, 2019 via email

Thomasdezeeuw commented Jul 21, 2019

carllerche commented Jul 21, 2019

Thomasdezeeuw commented Jul 21, 2019

Thomasdezeeuw commented Jul 21, 2019

PerfectLaugh commented Jul 21, 2019 via email

PerfectLaugh commented Jul 21, 2019 via email

Thomasdezeeuw commented Jul 21, 2019

carllerche commented Jul 21, 2019

Thomasdezeeuw commented Jul 22, 2019

Thomasdezeeuw commented Jul 22, 2019

Thomasdezeeuw commented Aug 3, 2019

Thomasdezeeuw commented Aug 6, 2019

PerfectLaugh commented Aug 6, 2019

Thomasdezeeuw commented Aug 13, 2019

carllerche left a comment

Remove net2 dependency #1029

Remove net2 dependency #1029

Conversation

Thomasdezeeuw commented Jul 11, 2019 • edited

carllerche left a comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Jul 11, 2019

carllerche commented Jul 15, 2019

Thomasdezeeuw commented Jul 16, 2019

Thomasdezeeuw commented Jul 21, 2019

Thomasdezeeuw Jul 21, 2019

Choose a reason for hiding this comment

Thomasdezeeuw commented Jul 21, 2019

Thomasdezeeuw commented Jul 21, 2019

carllerche commented Jul 21, 2019

PerfectLaugh commented Jul 21, 2019 via email

PerfectLaugh commented Jul 21, 2019 via email

carllerche commented Jul 21, 2019

carllerche commented Jul 21, 2019

PerfectLaugh commented Jul 21, 2019 via email

Thomasdezeeuw commented Jul 21, 2019

carllerche commented Jul 21, 2019

Thomasdezeeuw commented Jul 21, 2019

Thomasdezeeuw commented Jul 21, 2019

PerfectLaugh commented Jul 21, 2019 via email

PerfectLaugh commented Jul 21, 2019 via email

Thomasdezeeuw commented Jul 21, 2019

carllerche commented Jul 21, 2019

Thomasdezeeuw commented Jul 22, 2019

Thomasdezeeuw commented Jul 22, 2019

Thomasdezeeuw commented Aug 3, 2019

Thomasdezeeuw commented Aug 6, 2019

PerfectLaugh commented Aug 6, 2019

Thomasdezeeuw commented Aug 13, 2019

carllerche left a comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Jul 11, 2019 •

edited