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

feat(network): commonize raw socket interface #3221

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

Conversation

ArcticLampyrid
Copy link
Contributor

Subsystem
ktor-network

Motivation
Part of KTOR-5005
This PR didn't add impl for new platforms, but at least you can use the ASocket (and other interfaces) for commonMain sourceSet

Solution

  • Pull up some interfaces to commonMain sourceSet.
  • Make SelectorManager totally opaque in common sourceSet because it's highly platform-specific.
    • When we implement SelectorManager for JS (Node) in future, as Node.js always uses a global event loop, we may want to implement SelectorManager as a placeholder without any members.
    • When we implement SelectorManager for Windows (mingwX64), we may use IOCP model, which is quite different from the unix-style Select model, thus something like Selectable / interestOp may not be applicable.
    • For most platforms, having a manager (running a event loop, a event queue, etc.) to handle sockets is a reasonable abstract semantic.

Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

While I like an idea of commonization, I would think, that what is really need to be commonized here are *Socket classes without builders, as to make it possible to create future ktor-based or just external implementation of sockets (f.e. based on netty on JVM, or nodejs, or windows)

But, builders and selector semantics are not good candidates for commonization, as this is done differently in different platforms. Even now, as we have SelectorManager/Selectable shared between jvm and nix we can see, that almost no code is shared and only an entrypoint of creation SelectorManager is the same, and even then, context parameter is unused on nix.

dispatcher: CoroutineContext = EmptyCoroutineContext
): SelectorManager

public expect interface SelectorManager : CoroutineScope, Closeable {
Copy link
Contributor

Choose a reason for hiding this comment

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

What it the reason at all to provide this empty SelectorManager in common API, when f.e. nodejs sockets, can be implemented without it, as there is only single global event loop there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to support the platforms without default event loop, so we need a "manager". (maybe not selector manager. Actually I'm just following the origin name here since I didn't define any member related to Select on common)

For those platforms that only single global event loop is avaliable, it makes no sence itself. But if we want to write codes on common, we need a placeholder or we'll be hard on commonizing the user code.

Copy link
Contributor

Choose a reason for hiding this comment

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

my question is more about: why at all expose an interface in common, which has no sense on JS?
We can create another aSocket functions, which will not use selector manager in multiplatform code f.e.

Overall, I don't understand, why to start with this?
Exposing Socket classes - ok
Exposing Select
classes - not ok, as for now, it's more of an internal detail, rather then something, that could be used outside of ktor-network independent from ktor-network itself.

IMO: better to start trying to implement nodejs/mingw sockets, and see, if it's feasible to use SelectorManager abstraction, or may be it's better to create something new
Like instead of aSocket(selectorManager) expose something like aSocket(socketEngine) or something fully different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exposing Select classes - not ok, as for now, it's more of an internal detail, rather then something, that could be used outside of ktor-network independent from ktor-network itself.

Actually I did not treat it as a Selector (maybe I need to rename it), but a SocketDispatcher or NetworkEventLoopContext. (I believe such a context is needed for platforms that supports running several isolated event loops, and such interfaces need commonizing)

IMO: better to start trying to implement nodejs/mingw sockets, and see, if it's feasible to use SelectorManager abstraction, or may be it's better to create something new

I'll think twice about whether Interface Driven Development is suitable in this case.

@ArcticLampyrid
Copy link
Contributor Author

A common builder should be required since when you are writing connect to 192.168.1.114:514, send "Hello", you are not expected to write a platform-specific code to implement connect method.

In my abstract model, each async socket should be assigned to a Dispatcher. In some languages (like Node.js, Golang, etc), such a Dispatcher is a singleton object (created by the language runtime) shared for global usage. In other languages (like JVM-based languages, C/C++, Rust, etc), Dispatcher is created by user and we can have several parallel Dispatcher.
To support to use multiple Dispatcher, we need a builder that receive a Dispatcher as a parameter.

A specific socket usually requires a specific Dispatcher, so we need to either create a socket via Dispatcher (dispatcher.CreateConnection(...)) or having a common Builder but write something like

fun CreateConnection(dispatcher: SocketDispatcher) {
    if (!dispatcher is LinuxX64SocketDispatcher) throw Exception("...")
}

I believe the former is more friendly to type system.
BTW, semantically speaking, connect and bind are also a kind of tasks that require dispatching.

For multiple engines supporting, I believe it's out of scope in this issue. If it's needed, then maybe

val dispatcher = engine.CreateSocketDispatcher()
val socket = dispatcher.CreateConnection(address)
socket.openChannelForWriting().apply {
    ..
}

@ArcticLampyrid ArcticLampyrid marked this pull request as draft October 26, 2022 13:58
avoid publishing detailed `SelectorManager`
avoid publishing too specific Builder
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

2 participants