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.

Sign up for GitHub

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

Fix TCP connectivity on systems w/o IPv6 support #432

Merged
merged 6 commits into from
Feb 24, 2021
Merged

Fix TCP connectivity on systems w/o IPv6 support #432

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
103e884
Fix ipv4 support
jasper-d Feb 9, 2021
15cf100
Delete test app
jasper-d Feb 15, 2021
3071945
Fix tests
jasper-d Feb 15, 2021
e23ffef
Remove assertion
jasper-d Feb 15, 2021
4c8f05b
Remove superfluous using directive
jasper-d Feb 15, 2021
d563bd4
Fix whitespace
jasper-d Feb 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions src/NATS.Client/Conn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,10 @@ internal void open(Srv s, int timeoutMillis)
sslStream = null;
}

client = new TcpClient(AddressFamily.InterNetworkV6);
client.Client.DualMode = true;
client = new TcpClient(Socket.OSSupportsIPv6 ? AddressFamily.InterNetworkV6 : AddressFamily.InterNetwork);
if(Socket.OSSupportsIPv6)
jasper-d marked this conversation as resolved.
Show resolved Hide resolved
client.Client.DualMode = true;

var task = client.ConnectAsync(s.url.Host, s.url.Port);
// avoid raising TaskScheduler.UnobservedTaskException if the timeout occurs first
task.ContinueWith(t => GC.KeepAlive(t.Exception), TaskContinuationOptions.OnlyOnFaulted);
Expand Down Expand Up @@ -811,8 +813,9 @@ private void setupServerPool()
// createConn will connect to the server and wrap the appropriate
// bufio structures. It will do the right thing when an existing
// connection is in place.
private bool createConn(Srv s)
private bool createConn(Srv s, out Exception ex)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use ExceptionDispatchInfo so you can preserve the callstack below.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, ExceptionDispatchInfo.Throw(exToThrow) down below but that excludes .NET Framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, ExceptionDispatchInfo.Throw(exToThrow) down below but that excludes .NET Framework.

Unfortunately that's neither available in netcoreapp < 2.0 nor netstandard < 2.1.

Should probably use ExceptionDispatchInfo so you can preserve the callstack below.

Been there, reverted it. There are two problems with ExceptionDispatchInfo here:

  1. In the doReconnect path Connection.lastEx is set which is a raw exception. One could unwrap ExceptionDispatchInfo.SourceExcpetion in the reconnect code, but would probably loose the stack trace in the process. Changing Connection.lastEx to ExceptionDispatchInfo is not feasible, since it would result in a refactoring of large parts of Conn.cs and one would still hit some dead ends (i.e. ConnEventArgs exposes some exceptions though its Error property, so unwrapping there would be mandatory).
  2. Even when unwrapping in the doReconnect path, we couldn't just edi.Throw() because that would add a bunch of new exceptions which clients are not expected to handle right now. So we could either do something like:
if(exToThrow?.SourceException is NATSException) exToThrow.Throw();
if(exToThrow is object) throw new NATSConnectionException("Failed to connect", exToThrow.SourceException) // <-- some stack trace lost
throw new NATSNoServersException("");

or even worse

if(exToThrow?.SourceException is NATSException) exToThrow.Throw();
if(exToThrow is object) 
  try { exToThrow.Throw(); }
  catch (Exception ex) { throw new NATSConnectionException("Failed to connect", ex); } // <-- stack trace preserved but a kitten died
throw new NATSNoServersException("");

While it's not perfect, I think this implementation is an acceptable compromise. It provides better information regarding the actual error and keeps changes limited.
For reference stack traces of socket and timeout exceptions using the implementation in this PR and w/ EDI + unwrapping SourceExcpetion (may look different on legacy .NET):

This PR:

NATS.Client.NATSConnectionException: Failed to connect
---> System.Net.Sockets.SocketException (97): Address family not supported by protocol
   at System.Net.Sockets.Socket..ctor(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType)
   at System.Net.Sockets.TcpClient.InitializeClientSocket()
   at System.Net.Sockets.TcpClient..ctor(AddressFamily family)
   at NATS.Client.Connection.TCPConnection.open(Srv s, Int32 timeoutMillis) in /src/nats.net/src/NATS.Client/Conn.cs:line 380
   at NATS.Client.Connection.createConn(Srv s, Exception& ex) in /src/nats.net/src/NATS.Client/Conn.cs:line 823
   --- End of inner exception stack trace ---
   at NATS.Client.Connection.connect() in /src/nats.net/src/NATS.Client/Conn.cs:line 1186
   at NATS.Client.ConnectionFactory.CreateConnection(Options opts) in /src/nats.net/src/NATS.Client/ConnectionFactory.cs:line 171
   at IpTests.Program.Main(String[] args) in /src/nats.net/src/Samples/IpTests/Program.cs:line 27
   
NATS.Client.NATSConnectionException: timeout
   at NATS.Client.Connection.connect() in /src/nats.net/src/NATS.Client/Conn.cs:line 1185
   at NATS.Client.ConnectionFactory.CreateConnection(Options opts) in /src/nats.net/src/NATS.Client/ConnectionFactory.cs:line 171
   at IpTests.Program.Main(String[] args) in /src/nats.net/src/Samples/IpTests/Program.cs:line 27

With EDI + unwrapping:

NATS.Client.NATSConnectionException: Failed to connect
---> System.Net.Sockets.SocketException (97): Address family not supported by protocol
   at System.Net.Sockets.Socket..ctor(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType)
   at System.Net.Sockets.TcpClient.InitializeClientSocket()
   at System.Net.Sockets.TcpClient..ctor(AddressFamily family)
   at NATS.Client.Connection.TCPConnection.open(Srv s, Int32 timeoutMillis) in /src/nats.net/src/NATS.Client/Conn.cs:line 381
   at NATS.Client.Connection.createConn(Srv s, ExceptionDispatchInfo& ex) in /src/nats.net/src/NATS.Client/Conn.cs:line 824
   --- End of inner exception stack trace ---
   at NATS.Client.Connection.connect() in /src/nats.net/src/NATS.Client/Conn.cs:line 1187
   at NATS.Client.ConnectionFactory.CreateConnection(Options opts) in /src/nats.net/src/NATS.Client/ConnectionFactory.cs:line 171
   at IpTests.Program.Main(String[] args) in /src/nats.net/src/Samples/IpTests/Program.cs:line 27

NATS.Client.NATSConnectionException: timeout
   at NATS.Client.Connection.TCPConnection.open(Srv s, Int32 timeoutMillis) in /src/nats.net/src/NATS.Client/Conn.cs:line 395
   at NATS.Client.Connection.createConn(Srv s, ExceptionDispatchInfo& ex) in /src/nats.net/src/NATS.Client/Conn.cs:line 824
--- End of stack trace from previous location where exception was thrown ---
   at NATS.Client.Connection.connect() in /src/nats.net/src/NATS.Client/Conn.cs:line 1185
   at NATS.Client.ConnectionFactory.CreateConnection(Options opts) in /src/nats.net/src/NATS.Client/ConnectionFactory.cs:line 171
   at IpTests.Program.Main(String[] args) in /src/nats.net/src/Samples/IpTests/Program.cs:line 27

{
ex = null;
try
{
conn.open(s, opts.Timeout);
Expand All @@ -832,8 +835,9 @@ private bool createConn(Srv s)
bw = conn.getWriteBufferedStream(Defaults.defaultBufSize);
br = conn.getReadBufferedStream();
}
catch (Exception)
catch (Exception e)
{
ex = e;
return false;
}

Expand Down Expand Up @@ -1120,7 +1124,7 @@ internal bool connect(Srv s, out Exception exToThrow)
{
lock (mu)
{
if (!createConn(s))
if (!createConn(s, out exToThrow))
return false;

processConnectInit(s);
Expand Down Expand Up @@ -1175,10 +1179,11 @@ internal void connect()
{
if (status != ConnState.CONNECTED)
{
if (exToThrow == null)
exToThrow = new NATSNoServersException("Unable to connect to a server.");

throw exToThrow;
if (exToThrow is NATSException)
throw exToThrow;
if (exToThrow != null)
throw new NATSConnectionException("Failed to connect", exToThrow);
throw new NATSNoServersException("Unable to connect to a server.");
}
}
}
Expand Down Expand Up @@ -1617,7 +1622,7 @@ private void doReconnect()
try
{
// try to create a new connection
if(!createConn(cur))
if(!createConn(cur, out lastEx))
continue;
}
catch (Exception)
Expand Down
5 changes: 4 additions & 1 deletion src/Tests/IntegrationTests/TestBasic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1962,7 +1962,10 @@ public void TestNoEcho()
[Fact]
public void TestServersOption()
{
Assert.ThrowsAny<NATSNoServersException>(() => Context.ConnectionFactory.CreateConnection());
var exception = Record.Exception(() => Context.ConnectionFactory.CreateConnection());

Assert.IsType<NATSConnectionException>(exception);
Assert.Equal("timeout", exception.Message);
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Tests/IntegrationTests/TestCluster.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public void TestServersOption()
o.NoRandomize = true;
o.Servers = Context.GetTestServersUrls();

Assert.ThrowsAny<NATSNoServersException>(() => cf.CreateConnection(o));
Assert.ThrowsAny<NATSConnectionException>(() => cf.CreateConnection(o));

// Make sure we can connect to first server if running
using (NATSServer ns = NATSServer.Create(Context.Server1.Port))
Expand Down