close
Skip to content

Allow change default keep alive settings to run on Windows only - #232

Closed
quynh-ng wants to merge 4 commits into
statianzo:masterfrom
quynh-ng:master
Closed

Allow change default keep alive settings to run on Windows only#232
quynh-ng wants to merge 4 commits into
statianzo:masterfrom
quynh-ng:master

Conversation

@quynh-ng

Copy link
Copy Markdown

The method "Socket.IOControl" is only available on Windows platform, we need to check before using its.

@AppVeyorBot

Copy link
Copy Markdown

Build Fleck 0.0.21-ci failed (commit 0874b2a78d by @quynh-ng)

1 similar comment
@AppVeyorBot

Copy link
Copy Markdown

Build Fleck 0.0.21-ci failed (commit 0874b2a78d by @quynh-ng)

@darkl

darkl commented May 30, 2018

Copy link
Copy Markdown
Collaborator

Hi @statianzo, @Silvenga,
This change is important in order to allow Fleck to run over Unix based systems, as the method Socket.IOControl is Windows specific.

The build fails but it has completely nothing to do with this pull request. @Silvenga put an IgnoreAttribute above the failing tests with an Until = "2018-04-29" property, and that date has already passed.

Please pay attention to this pull request and consider merging it.

Thanks
Elad

@statianzo

Copy link
Copy Markdown
Owner

Thanks for the reminder @darkl
@quynh-ng if you could make those failing tests into just Ignore without until. I can get this merged and a new version released

@Silvenga

Copy link
Copy Markdown
Collaborator

I was concerned with how reliable the portability analyzer was when I started the port (it recorded 100% compatibility), I guess it turns out it doesn't check for everything.

@statianzo, are you okay with me updating AppVeyor build to test under Linux as well? https://www.appveyor.com/docs/getting-started-with-appveyor-for-linux/

@statianzo

Copy link
Copy Markdown
Owner

@statianzo, are you okay with me updating AppVeyor build to test under Linux as well?

Of course. That sounds like a useful environment to test on.

@quynh-ng

Copy link
Copy Markdown
Author

I ran the test on Ubuntu Linux and macOS High Sierra, the test fails with some reasons, when I looked at the results, I found 2 reasons that related to the runtime:

  1. SocketWarpper.cs - line 51 => socket.IOControl(...): System.PlatformNotSupportedException : Socket.IOControl handles Windows-specific control codes and is not supported on this platform.

  2. WebSocketServer.cs - line 28 => socket.SetSocketOption(SocketOptionLevel.IPv6, SocketOptionName.IPv6Only, false);

  • Linux: System.Net.Sockets.SocketException : Protocol not available
  • macOS: System.Net.Sockets.SocketException : Invalid argument

And I really don't known why the build is failed.

What should I really need to do?
Do I need to re-create other pull request?

@AppVeyorBot

Copy link
Copy Markdown

Build Fleck 0.0.22-ci failed (commit 789c4cb55e by @quynh-ng)

1 similar comment
@AppVeyorBot

Copy link
Copy Markdown

Build Fleck 0.0.22-ci failed (commit 789c4cb55e by @quynh-ng)

@darkl

darkl commented May 31, 2018

Copy link
Copy Markdown
Collaborator

Remove these Until properties to make the build pass #1 #2.

Elad

@AppVeyorBot

Copy link
Copy Markdown

Build Fleck 0.0.23-ci failed (commit 823abbd86c by @quynh-ng)

1 similar comment
@AppVeyorBot

Copy link
Copy Markdown

Build Fleck 0.0.23-ci failed (commit 823abbd86c by @quynh-ng)

@quynh-ng

Copy link
Copy Markdown
Author

@darkl Thanks Elad.

Two ignore test are pass, but still got error on build:

Fleck.Tests.WebSocketServerTests.ShouldSupportDualStackListenWhenServerV6All
System.Net.Sockets.SocketException : No connection could be made because the target machine actively refused it 127.0.0.1:8000
at System.Net.Sockets.Socket.DoConnect(EndPoint endPointSnapshot, SocketAddress socketAddress)
at System.Net.Sockets.Socket.Connect(EndPoint remoteEP)
at System.Net.Sockets.Socket.Connect(IPAddress address, Int32 port)
at Fleck.Tests.WebSocketServerTests.ShouldSupportDualStackListenWhenServerV6All()

I don't get it...

@AppVeyorBot

Copy link
Copy Markdown

Build Fleck 0.0.24-ci completed (commit 229b87de1b by @quynh-ng)

1 similar comment
@AppVeyorBot

Copy link
Copy Markdown

Build Fleck 0.0.24-ci completed (commit 229b87de1b by @quynh-ng)

@statianzo

Copy link
Copy Markdown
Owner

Nice work on getting the build green again. Can the change can be done without replacing spaces with tabs? It's difficult to tell what was actually changed in the git history and looks like a whole file rewrite.

@quynh-ng

Copy link
Copy Markdown
Author

I think the file is reformated by Visual Studio.

Here my update, only 2 files just 10 lines of code

SocketWrapper.cs:

Line 10: using System.Runtime.InteropServices;

Line 65 - 70:

SetKeepAlive(socket, KeepAliveInterval, RetryInterval);

===>

#if NET45

		SetKeepAlive(socket, KeepAliveInterval, RetryInterval);

#else

		if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))

			SetKeepAlive(socket, KeepAliveInterval, RetryInterval);

#endif

WebSocketServer.cs:

Line 8: using System.Runtime.InteropServices;

Line 29 - 30:

#else

socket.SetSocketOption(SocketOptionLevel.IPv6, SocketOptionName.IPv6Only, false);

==>

#elif !NET45

			if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))

				socket.SetSocketOption(SocketOptionLevel.IPv6, SocketOptionName.IPv6Only, false);

darkl added a commit to darkl/Fleck that referenced this pull request May 31, 2018
@darkl darkl mentioned this pull request May 31, 2018
@darkl

darkl commented May 31, 2018

Copy link
Copy Markdown
Collaborator

@statianzo I made the described changes in #233.

Elad

statianzo pushed a commit that referenced this pull request May 31, 2018
The method "Socket.IOControl" is only available on Windows platform, we need to check before using its.

This pull request contains the changes described in #232, with no tab/space changes.
@statianzo

Copy link
Copy Markdown
Owner

Merged #233. Thank you for the help @quynh-ng!

@statianzo statianzo closed this May 31, 2018
@statianzo

Copy link
Copy Markdown
Owner

Released on NuGet as 1.0.3

@darkl

darkl commented May 31, 2018

Copy link
Copy Markdown
Collaborator

Thanks!

localcc pushed a commit to localcc/Fleck that referenced this pull request Oct 16, 2021
…ianzo#233)

The method "Socket.IOControl" is only available on Windows platform, we need to check before using its.

This pull request contains the changes described in statianzo#232, with no tab/space changes.
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

Successfully merging this pull request may close these issues.

5 participants