Skip to content

Socket Connection Factory#828

Merged
mtmk merged 6 commits into
mainfrom
socket-conn-factory
Apr 15, 2025
Merged

Socket Connection Factory#828
mtmk merged 6 commits into
mainfrom
socket-conn-factory

Conversation

@caleblloyd

Copy link
Copy Markdown
Collaborator

@caleblloyd
caleblloyd requested a review from mtmk April 14, 2025 05:56
Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com>
@caleblloyd
caleblloyd force-pushed the socket-conn-factory branch from 21d7b43 to 5465c4a Compare April 14, 2025 06:05
@wolfkor

wolfkor commented Apr 14, 2025

Copy link
Copy Markdown
Contributor

This is more in line with my understanding of a factory, but it changes the API ;-). I tried it with an implementation of INatsSocketConnectionFactory and INatsTlsUpgradeSocketConnection, and it seems to work. But shouldn't it be called INatsProxyUpgradeSocketConnection?

@caleblloyd

Copy link
Copy Markdown
Collaborator Author

This is more in line with my understanding of a factory, but it changes the API ;-

Yes, it seemed more natural to use an interface that the internal and external implementations could both implement

Shouldn't it be called INatsProxyUpgradeSocketConnection

I don't think that it is specific to proxies, you could implement other kinds of sockets like Unix sockets too

@wolfkor

wolfkor commented Apr 14, 2025

Copy link
Copy Markdown
Contributor

but also not specific to TLS ;-)

Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com>
@caleblloyd

Copy link
Copy Markdown
Collaborator Author

INatsSocketConnection is the one that is not-specefic to TLS, the library will not attempt to do a TLS upgrade on that socket.

But INatsTlsUpgradeableSocketConnection (which extends INatsSocketConnection) will cause the library to attempt to do a TLS upgrade if one is warranted.

@caleblloyd

caleblloyd commented Apr 14, 2025

Copy link
Copy Markdown
Collaborator Author

Tests are now passing but I think we should explore 2 more areas:

  1. Pull Connection Timeout logic up a level so that each implementation doesn't have to handle it
  2. Evaluate if we can simplify the INatsSocketConnection interface to get SignalDisconnected + WaitForClosed out of it

Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com>
Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com>
@caleblloyd

Copy link
Copy Markdown
Collaborator Author

I reduced INatsSocketConnection to SendAsync, ReceiveAsync, and DisposeAsync. Closing the socket is now done with DisposeAsync

Comment thread src/NATS.Client.Core/Internal/TcpConnection.cs
Comment thread src/NATS.Client.Core/NatsConnection.cs

@mtmk mtmk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome feature! LGTM thanks @caleblloyd

@mtmk
mtmk merged commit 95456b7 into main Apr 15, 2025
@mtmk
mtmk deleted the socket-conn-factory branch April 15, 2025 11:41
mtmk added a commit that referenced this pull request Apr 16, 2025
* Stopped testing with net6.0 (#821)
* Update JetStream models (#832)
* Add direct request-reply (#829)
* Socket Connection Factory (#828)
* Fix proxy with TLS scenario with SocketConnectionFactory (#826)
* Make NatsJSConstants public (#825)
* Use Stopwatch in PingCommand for more precise timing (#827)
* Fix netstandard bugs and tidy up tests (#817)
@mtmk mtmk mentioned this pull request Apr 16, 2025
mtmk added a commit that referenced this pull request Apr 16, 2025
* Stopped testing with net6.0 (#821)
* Update JetStream models (#832)
* Add direct request-reply (#829)
* Socket Connection Factory (#828)
* Fix proxy with TLS scenario with SocketConnectionFactory (#826)
* Make NatsJSConstants public (#825)
* Use Stopwatch in PingCommand for more precise timing (#827)
* Fix netstandard bugs and tidy up tests (#817)
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.

Communication with TLS bus behind proxy results in SslPolicyErrors.RemoteCertificateNameMismatch

3 participants