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

Review client support for Unix sockets #3180

Closed
sbordet opened this issue Dec 3, 2018 · 4 comments
Closed

Review client support for Unix sockets #3180

sbordet opened this issue Dec 3, 2018 · 4 comments

Comments

@sbordet
Copy link
Contributor

sbordet commented Dec 3, 2018

The HttpClient support for Unix socket needs to be reviewed.

Currently, it is implemented as a transport for HttpClient, but Unix sockets are really a replacement for network sockets, rather than a new way to encode HTTP requests and responses over the "wire", as Unix sockets support TCP (among other protocols, included UDP).

The current implementation also only opens just one socket for the whole transport, and inherits from the HTTP/1.1 transport.
I don't think this is correct, as it won't be possible to send concurrent requests over a single Unix socket, as much as it's not possible with a single network socket.
And there is nothing that forbids a Unix socket to transport HTTP/2 or FastCGI, rather than only HTTP/1.1, while now HTTP/1.1 is hardcoded.

Class UnixEndPoint seems unnecessary, as JNR returns a SocketChannel so the current SocketChannelEndPoint is enough.

The only difference for Unix sockets is really how the java.nio.Selector is created, as it needs to plug in the JNR implementation.

In light of #132 and #1350 - i.e. having a single HttpClient be able to speak multiple protocols dynamically, we need one more degree of freedom: whether to open a network socket or a Unix socket for a given destination.
But perhaps this degree of freedom is another issue.

@sbordet
Copy link
Contributor Author

sbordet commented Dec 3, 2018

See also #1881, #2014, #2634, #3136.

@joakime
Copy link
Contributor

joakime commented Dec 3, 2018

Class UnixEndPoint seems unnecessary

There are SocketAddress differences that need to be handled correctly.
JNR implements SocketAddress (via UnixSocketAddress implementation), not InetSocketAddress like all of our other EndPoint implementations.

@sbordet
Copy link
Contributor Author

sbordet commented Dec 3, 2018

@joakime perhaps, but I think it's enough a ternary expression in SocketChannelEndPoint when retrieving the addresses.

sbordet added a commit that referenced this issue Mar 19, 2019
Reviewed the implementation.
Got rid of the single channel stored in the HttpClientTransport.
Re-enabled tests on the Unix socket transport.
Updated JNR to 0.22.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 19, 2019
Fixed tests on Jenkins by shortening the Unix socket file name.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 19, 2019
Updated code after reviews.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Apr 6, 2019
Issue #3180 - Review client support for Unix sockets.
@sbordet
Copy link
Contributor Author

sbordet commented Nov 28, 2019

Addressed in #3474.

@sbordet sbordet closed this as completed Nov 28, 2019
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

2 participants