Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

NIO: excise UDS support for Windows #1404

Merged
merged 2 commits into from
Feb 18, 2020

Conversation

compnerd
Copy link
Contributor

This removes support for Unix Domain Sockets from NIO on Windows.
Although Windows 10.17063 introduced basic support for UDS, the support
is incomplete and has differences from the tradtional UDS sockets in
terms of behaviour. Excise the support initially to get NIO to build on
Windows.

[One line description of your change]

Motivation:

[Explain here the context, and why you're making that change. What is the problem you're trying to solve.]

Modifications:

[Describe the modifications you've done.]

Result:

[After your change, what will change.]

@swift-server-bot
Copy link

Can one of the admins verify this patch?

9 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@compnerd
Copy link
Contributor Author

One question that comes to mind: should this be a general option rather than platform conditional? I imagine that WASM may not implement UDS either and so more than just windows would want the UDS support made optional.

This removes support for Unix Domain Sockets from NIO on Windows.
Although Windows 10.17063 introduced basic support for UDS, the support
is incomplete and has differences from the tradtional UDS sockets in
terms of behaviour.  Excise the support initially to get NIO to build on
Windows.
@compnerd compnerd force-pushed the unix-is-not-in-the-domain branch from 6c60ef5 to 469c92c Compare February 18, 2020 07:43
@Lukasa
Copy link
Contributor

Lukasa commented Feb 18, 2020

I imagine that WASM may not implement UDS either and so more than just windows would want the UDS support made optional.

WASM will want a lot more removed than just UDS, I think: NIO won't function at all on WASM as it relies on far too much of the system.

When WASM becomes something we really want to worry about we may need to consider a substantial refactor of the APIs. In particular, we probably will need to separate the bits of NIO that are truly platform agnostic (things like protocols, ChannelPipeline, ChannelHandler, various helper objects) from the things that are actually secretly "NIOPOSIX" (SelectableEventLoop, MultithreadedEventLoopGroup, and so-on).

I don't think we want to go too far down that road today, because it's a pretty substantial breaking change, but if we want to get serious about WASM it'll necessarily come up.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

LGTM, seems reasonable.

@Lukasa Lukasa requested a review from weissi February 18, 2020 09:17
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Feb 18, 2020
@Lukasa Lukasa added this to the 2.15.0 milestone Feb 18, 2020
@Lukasa
Copy link
Contributor

Lukasa commented Feb 18, 2020

I'm marking this as a Semver patch because, well, we never worked on Windows before so this can't break existing APIs.

@Lukasa
Copy link
Contributor

Lukasa commented Feb 18, 2020

@swift-nio-bot test this please

@Lukasa Lukasa merged commit ca9f613 into apple:master Feb 18, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants