Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TcpKeepAliveSettings doesn't work on Linux #398

Closed
Havret opened this issue Feb 11, 2020 · 10 comments
Closed

TcpKeepAliveSettings doesn't work on Linux #398

Havret opened this issue Feb 11, 2020 · 10 comments

Comments

@Havret
Copy link
Contributor

Havret commented Feb 11, 2020

I am getting the following exception when, I try to use TcpKeepAliveSettings with the app deployed on Linux box. I'm not sure but this should be supported dotnet/corefx#29963. Do we need to make some adjustments in AmqpNetLite to make this work properly on Linux?

Unhandled exception. Apache.NMS.NMSException: Socket.IOControl handles Windows-specific control codes and is not supported on this platform.
---> System.PlatformNotSupportedException: Socket.IOControl handles Windows-specific control codes and is not supported on this platform.
   at System.Net.Sockets.SocketPal.WindowsIoctl(SafeSocketHandle handle, Int32 ioControlCode, Byte[] optionInValue, Byte[] optionOutValue, Int32& optionLength)
   at System.Net.Sockets.Socket.IOControl(Int32 ioControlCode, Byte[] optionInValue, Byte[] optionOutValue)
   at System.Net.Sockets.Socket.IOControl(IOControlCode ioControlCode, Byte[] optionInValue, Byte[] optionOutValue)
   at Amqp.SocketExtensions.SetTcpKeepAlive(Socket socket, TcpKeepAliveSettings settings)
   at Amqp.TcpSettings.Configure(Socket socket)
   at Amqp.TcpTransport.ConnectAsync(Address address, ConnectionFactory factory)
   at Amqp.ConnectionFactory.CreateAsync(Address address, Open open, OnOpened onOpened, IHandler handler)
   at Apache.NMS.AMQP.Provider.Amqp.AmqpConnection.Start()
   at Apache.NMS.AMQP.NmsConnection.CreateNmsConnection()
   --- End of inner exception stack trace ---
   at Apache.NMS.AMQP.NmsConnection.CreateNmsConnection()
   at Apache.NMS.AMQP.NmsConnection.CreateSession(AcknowledgementMode acknowledgementMode)
@xinchen10
Copy link
Member

netstandard1.3 or 2.0? 1.3 is still using very old 4.1.0 System.Net.Sockets package released in 2016. The fix was made in 2018. Have you tried 2.0 with the latest .net core runtime?

@HavretGC
Copy link

I think I'm using the new version. Here you can have a look at the app I sent to Red Hat to help them reproduce the issue --> https://github.com/HavretGC/TcpKeepAliveSettings

@HavretGC
Copy link

HavretGC commented Feb 19, 2020

Maybe we could address this the same way as it was done for SqlClient? --> dotnet/SqlClient#395

@xinchen10
Copy link
Member

That requires us to build multiple targets and lose the benefit of targeting just netstandard. This is what I have found.

Branch socketopt has the changes based on the above assumption. Can you try it on .Net Core 3.0+ runtime on Linux?

@Havret
Copy link
Contributor Author

Havret commented Feb 21, 2020

I was able to run my test app with the changes from socketopt branch on NET Core 3.1 runtime on Linux box. It seems to work, or at least it doesn't throw the exception anymore. I'm not sure how can I make sure that TcpKeepAliveSettings are applied and work as expected though. Probably that would require some packet sniffing with Wireshark or sth like that.

Possibly we could ask guys from Red Hat, to confirm that the fix is ok.

@Havret
Copy link
Contributor Author

Havret commented Feb 25, 2020

@xinchen10 as per Red Hat:

I did some more digging and review of NET core runtime code and the code given in the patch seems like it ought to work in cases where the platform supports those options. The code would probably still fail with a platform exception if the OS in question doesn't such as older versions of Windows 10 or a linux distro where the TCP specific options they are using from "/usr/include/netinet/tcp.h" which is what the net core uses via a CMake check to determine if those options are present.

Is that helpful ? If not what more information you need. Being specific to environment, it wouldn't be official fix.

So do we need anything more carry on with that?

@michaelpearce-gain
Copy link

LGTM to be honest.

@xinchen10
Copy link
Member

I have merged the change to master.

@HavretGC
Copy link

Can we expect the release any time soon? :)

@xinchen10
Copy link
Member

Released 2.4.0.

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

No branches or pull requests

4 participants