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

Jump to bottom

NIOConcurrencyHelpers: port to Windows #1403

Merged
merged 2 commits into from
Mar 3, 2020
Merged

NIOConcurrencyHelpers: port to Windows #1403

merged 2 commits into from
Mar 3, 2020

Conversation

compnerd
Copy link
Contributor

Add support for Windows to the NIOConcurrencyHelpers, replacing the
pthreads usage for Windows threading primitives.

[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
compnerd commented Feb 18, 2020
View reviewed changes
Sources/NIOConcurrencyHelpers/lock.swift Outdated
@@ -160,6 +204,9 @@ public final class ConditionLock<T: Equatable> {
public func lock(whenValue wantedValue: T, timeoutSeconds: Double) -> Bool {
precondition(timeoutSeconds >= 0)

#if os(Windows)
fatalError()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be ported over, but cannot be accurate as SleepConditionVariableSRW takes a relative time rather than absolute time. It also would have a +/-10ns time-frame as timers on Windows have a 10ns granularity.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I'm overly worried about timer granularity, so I don't really consider that to be a problem. I'm not sure I follow the relative vs absolute time concerns though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just that the sleep will not be exactly precise. I will have some overheads due to adjustments for the remaining sleep just in case of a spurious wakeup.

@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
Lukasa previously requested changes Feb 18, 2020
View reviewed changes
Sources/NIOConcurrencyHelpers/lock.swift Outdated
@@ -160,6 +204,9 @@ public final class ConditionLock<T: Equatable> {
public func lock(whenValue wantedValue: T, timeoutSeconds: Double) -> Bool {
precondition(timeoutSeconds >= 0)

#if os(Windows)
fatalError()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I'm overly worried about timer granularity, so I don't really consider that to be a problem. I'm not sure I follow the relative vs absolute time concerns though.

@compnerd compnerd marked this pull request as ready for review February 29, 2020 17:33
@compnerd compnerd changed the title [WIP] NIOConcurrencyHelpers: port to Windows NIOConcurrencyHelpers: port to Windows Feb 29, 2020
@compnerd compnerd requested a review from Lukasa February 29, 2020 21:40
weissi
weissi requested changes Mar 3, 2020
View reviewed changes
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Thank you! Looks mostly good, this there’s a synchronisation problem though

@compnerd compnerd force-pushed the concurrent-windows branch from 55d1d5f to 67c62c6 Compare March 3, 2020 16:35
@compnerd
NIOConcurrencyHelpers: port to Windows
cd723ee 
Add support for Windows to the `NIOConcurrencyHelpers`, replacing the
`pthreads` usage for Windows threading primitives.
@compnerd compnerd force-pushed the concurrent-windows branch from 67c62c6 to cd723ee Compare March 3, 2020 18:15
weissi
weissi approved these changes Mar 3, 2020
View reviewed changes
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@weissi weissi merged commit 5e728b5 into apple:master Mar 3, 2020
@compnerd compnerd deleted the concurrent-windows branch March 3, 2020 19:19
pull bot pushed a commit to scope-demo/swift-nio that referenced this pull request Mar 3, 2020
@compnerd @weissi @pull
NIOConcurrencyHelpers: port to Windows (apple#1403)
3006364 
Add support for Windows to the `NIOConcurrencyHelpers`, replacing the
`pthreads` usage for Windows threading primitives.

Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@weissi weissi weissi approved these changes

@Lukasa Lukasa Awaiting requested review from Lukasa

Assignees
No one assigned
Labels
🔨 semver/patch No public API change.
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@compnerd @swift-server-bot @weissi @Lukasa