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

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

Merged
merged 6 commits into from
Feb 24, 2021

Conversation

jasper-d
Copy link
Contributor

@jasper-d jasper-d commented Feb 15, 2021

  • Fix TCP connectivity
  • Throw specific exceptions in case of a connection failure
    • please review carefully, there's some global state involved that makes me feel uncomfortable

Fixes #431

src/NATS.Client/Conn.cs Outdated Show resolved Hide resolved
@@ -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

Co-authored-by: Christopher Watford <[email protected]>
Copy link
Member

@ColinSullivan1 ColinSullivan1 left a comment

Choose a reason for hiding this comment

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

This looks good to me. @watfordgnf - look OK?

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.

TCP connectivity broken on some systems
3 participants