Tcp Keepalive for Linux (NET8.0) #1120
Conversation
New Target .NET 8.0 Fixes setup of Tcp Keepalive under Linux
chrbauer
force-pushed
the
linux/tcp-keepalive-for-net80
branch
from
f7818c5 to
1c4add8
Compare
|
I think adding targets to all currently supported net targets (so 8/9) would probably be a good thing, it should probably be done in another PR. I maintain a fork since activity had slowed down on the repository and I wanted to "modernize" the repository a bit, I ended up completing the work of #1073 (and a bunch of smaller open PR) which I think is the right way about adding support to net target. More precisely, a lot of the changes in this PR use the pragma It would be very nice to keep some of the momentum going from #1111 |
|
Modernizing the project is a great idea. Even net6 had reached eol. The NETSTANDARD2_1_OR_GREATER could be indeed used on many places, but but not everywhere. The Pull request solves the core problem only for net8. I would like to have this merged. Unsure how to proceed. |
src/NetMQ.Tests/ClientServer.cs
Outdated
| @@ -82,7 +82,7 @@ public async void AsyncWithCancellationToken() | |||
| await Assert.ThrowsAnyAsync<OperationCanceledException>(async () => await server.ReceiveStringAsync(source.Token)); | |||
| } | |||
|
|
|||
| #if NETCOREAPP3_1 | |||
| #if NET8_0_OR_GREATER | |||
There was a problem hiding this comment.
IAsyncEnumerable earliest's availability is on Net Standard 2.1, I believe this check to be too restrictive in theory (although this is just a test project).
It should probably be either #if NET or #if NETSTANDARD2_1_OR_GREATER?
src/NetMQ/Annotations.cs
Outdated
| @@ -1,6 +1,6 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE.md file in the project root for more information. | |||
|
|
|||
| #if !NETSTANDARD2_1 | |||
| #if !NETSTANDARD2_1 && !NET8_0_OR_GREATER | |||
There was a problem hiding this comment.
Since this is to "stub in" attributes that only available on later frameworks, we probably should only target the missing frameworks?
Should probably be #if NET_FRAMEWORK?
src/NetMQ/AsyncReceiveExtensions.cs
Outdated
| @@ -1,4 +1,4 @@ | |||
| #if NETSTANDARD2_0 || NETSTANDARD2_1 || NET47 | |||
| #if NET8_0_OR_GREATER || NET8_0_OR_GREATER || NETSTANDARD2_0 || NETSTANDARD2_1 || NET47 | |||
There was a problem hiding this comment.
NET8_0_OR_GREATER was added twice.
I also believe a more "generic" statement like #if NETSTANDARD2_0_OR_GREATER || NET47_OR_GREATER would target all compatible frameworks and be "future" proof (won't need to review pragma if like more .NetFramework come out)
| @@ -157,7 +157,7 @@ PullMsgResult ProduceWelcome(ref Msg msg) | |||
| // 8-byte prefix plus 16-byte random nonce | |||
| CookieNoncePrefix.CopyTo(cookieNonce); | |||
| using var rng = RandomNumberGenerator.Create(); | |||
| #if NETSTANDARD2_1 | |||
| #if NET8_0_OR_GREATER || NETSTANDARD2_1 | |||
There was a problem hiding this comment.
Spans are available in all non-NetFramework and NetStandard2.1, so it should probably be #if NETSTANDARD2_1_OR_GREATER
| @@ -248,6 +248,13 @@ public void OutCompleted(SocketError socketError, int bytesTransferred) | |||
| // Set the TCP keep-alive option values to the underlying socket. | |||
| m_s.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, m_options.TcpKeepalive); | |||
|
|
|||
|
|
|||
| #if NET8_0_OR_GREATER | |||
There was a problem hiding this comment.
Last comment in the same "general" idea, these options became available in net5/netcore3.1, might be worth to do a #if NET here?
There was a problem hiding this comment.
I used the #if NET idea and made a commit
I'm in favor of adding net8/9 targets in additional to .net standard, although it should probably be done in its own PR and when the targets are added, then rebase this PR to only add the patch for the keep alive in the socket classes? |
|
I will prepare an update to set also TcpKeepaliveCnt... |
|
This was done last week |
New Target .NET 8.0
Fixes setup of Tcp Keepalive under Linux