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

KTOR-4766 - Add basic support to use UnixSockets with CIO #3342

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hfhbd
Copy link
Contributor

@hfhbd hfhbd commented Jan 5, 2023

Subsystem
CIO Client and Server

Motivation
Add basic support for unix sockets with a custom CIO client and server. Native (and better) support would require breaking changes.

Solution
Extract resolving the socketAddress into new functions without breaking changes. A user can now implement unix socket support using a custom cio client/server with minimal code.

@hfhbd hfhbd force-pushed the hfhbd/unix-sockets-cio branch 2 times, most recently from 4aa20ec to d81391a Compare January 5, 2023 12:43
@rsinukov rsinukov requested a review from e5l January 5, 2023 16:22
@hfhbd hfhbd force-pushed the hfhbd/unix-sockets-cio branch 2 times, most recently from eafe8a8 to e43245b Compare January 6, 2023 07:53
@e5l
Copy link
Member

e5l commented Aug 9, 2023

Hey @hfhbd, sorry for the delay. Could you rebase it on the main branch? I will take a look

@hfhbd
Copy link
Contributor Author

hfhbd commented Aug 9, 2023

You do accept breaking changes now, don't you?

@e5l
Copy link
Member

e5l commented Aug 9, 2023

Yep, we do :)

timeout: WeakTimeoutQueue,
serverJobName: CoroutineName,
acceptJobName: CoroutineName,
selector: SelectorManager,

Choose a reason for hiding this comment

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

This argument can be eliminated. Right now it's only for:

    serverJob.invokeOnCompletion {
        selector.close()
    }

but serverJob is available in the returned HttpServer instance as rootServerJob.

So the caller can do

val selector = ...
val server = httpServer(...)
server.rootServerJob.invokeOnCancelation {
  selector.cancel()
}

@JakeWharton
Copy link

I would absolutely love the new httpServer factory function to be made available so that I can use unix domain sockets in the abstract namespace (i.e., ones which do not appear on the filesystem).

Can we merge that separately from the CIO application stuff?

I would love to discuss propagating support for unix domain sockets into the public API, but also a non-internal API for wrapping arbitrary Socket sources.

@e5l
Copy link
Member

e5l commented Oct 27, 2023

Hey @JakeWharton, thanks for the feedback!

Sure, it would be great! We already have some basic Unix socket API in the ktor-network module and would be happy to have some feedback and cases we're missing.

Could you also tell me what you mean by separating the HTTP server from the CIO application? At the moment, the CIO is responsible for making HTTP over sockets.

@e5l e5l self-assigned this Oct 27, 2023
@JakeWharton
Copy link

The built-in unix domain socket support relies on UnixDomainSocketAddress API which is specific to the JVM (on Java 16+), not available on Android, and does not support usage of the abstract namespace (i.e., one that does not have a path) or unnamed.

Immediately, I am trying to wrap Android's LocalServerSocket to host a server on a unix domain socket in the abstract namespace. With the proposed changes in this PR, the () -> Socket factory lambda would should be sufficient to let me accomplish this. At least, to get an HttpServer. I'm not actually sure if that's enough to get everything working, as the concept of addresses is pervasive throughout the API.

In general, I would like to see setup of the transport sufficiently decoupled from the actual HTTP server so that I can set up whatever mechanism I want. A litmus test for this being successful would be using a file watcher on a folder that has requests/ and responses/ and responding to new text files in requests/ as HTTP requests by writing the associated HTTP response as a same-named file in responses/. Is this a completely contrived and useless mechanism in practice? Absolutely. But it would demonstrate the right level of decoupling of the layers such that I don't have to worry about the use of transport setups as yet unknown to ktor being a blocker.

Could you also tell me what you mean by separating the HTTP server from the CIO application? At the moment, the CIO is responsible for making HTTP over sockets.

Sorry I was a little imprecise here. I was referring to the CIO value usually supplied to embeddedServer, or the associated setup of a CIOApplicationEngine which seems firmly tied to TCP or UDP and addresses as a transport mechanism. There is no non-internal API that would let me create a custom transport on top of the CIO HTTP engine. In the end of all this I would like to be able to do embeddedServer(CIOLocalServerSocket(name)) { .. } where CIOLocalServerSocket is a type I write to adapt Android's LocalServerSocket and LocalSocket APIs to ktor-network's ServerSocket and Socket APIs.

@JakeWharton
Copy link

I'll also add that the client should ideally support similar levels of layering if it doesn't already. What use an HTTP server listening on an UDS in the abstract namespace if I can't also send it requests from the client using that mechanism as its transport?

@e5l
Copy link
Member

e5l commented Oct 27, 2023

@JakeWharton, thanks for the clarification!

Got it! It looks possible. Let me play with it and check what API needs to be public and how we can expose it. It seems it should be all done on the ktor-network level (both for client and server).

Btw if you already have an idea of how to do this, please let me know

@JakeWharton
Copy link

Prior to this my usage of ktor server has never exceeded the most basic setup of one or two routes. I'm just now starting to investigate more wild usages so still trying to learn the structure of the internals.

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.

None yet

3 participants