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.

Sign up for GitHub

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

Jump to bottom

Migrate from Lock to NIOLock #2891

Merged
merged 6 commits into from Oct 13, 2022
Merged

Migrate from Lock to NIOLock #2891

merged 6 commits into from Oct 13, 2022

Conversation

MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Oct 4, 2022 •
edited by 0xTim

Lock has recently been deprecated in favor of NIOLock.

Changes

All Locks have been renamed to NIOLock.

@MahdiBM MahdiBM changed the title Move to from Lock to NIOLock Move from Lock to NIOLock Oct 4, 2022
@MahdiBM
Copy link
Contributor Author

MahdiBM commented Oct 4, 2022 •
edited

The test failures seem real, but also unrelated to this PR.
ServerTests.testConfigureHTTPDecompressionLimit seems to consistently fail. Perhaps due to a change in NIO-Extras?

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Oct 4, 2022 •
edited

Opened an issue for the test failures: #2892

0xTim
0xTim requested changes Oct 13, 2022
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

I don't think we need to change any of the names of the application properties since they're namespaced under Application

Sources/Vapor/Application.swift Outdated
@@ -46,13 +48,45 @@ public final class Application {
}
}
}

public final class NIOLocks {
Copy link
Member

Choose a reason for hiding this comment

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

Since this is Application.Lock I don't think we actually need to rename any of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you elaborate more please?
there is rule where if public properties are name-spaced under another public property, we can make breaking changes to them?!
or do you mean I could have fit NIOLock stuff under the same Locks class?

Copy link
Member

Choose a reason for hiding this comment

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

We only need to change the deprecated types (so Lock to NIOLock) rather than renaming everything that refers to them

Copy link
Contributor Author

@MahdiBM MahdiBM Oct 13, 2022 •
edited

Choose a reason for hiding this comment

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

thats news to me from what I understand ... does this new commit fix the problem?

0xTim
0xTim requested changes Oct 13, 2022
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Yep this is looking good! Once last change then good to go

Sources/Vapor/Application.swift Outdated
@@ -71,7 +72,6 @@ public final class Application {
case .createNew:
self.eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: System.coreCount)
}
self.locks = .init()
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this back to reduce the change and make it easier to work out where it's created

Sources/Vapor/Application.swift Outdated
Comment on lines 50 to 51
public var locks: Locks
public var locks = Locks()
Copy link
Member

Choose a reason for hiding this comment

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

Revert

Copy link
Contributor Author

@MahdiBM MahdiBM Oct 13, 2022 •
edited

Choose a reason for hiding this comment

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

👍 had to do it before to avoid deprecation warnings

@0xTim 0xTim added the semver-patch Internal changes only label Oct 13, 2022
@0xTim 0xTim changed the title Move from Lock to NIOLock Migrate from Lock to NIOLock Oct 13, 2022
@0xTim
Copy link
Member

0xTim commented Oct 13, 2022

Original Comment:
Lock has recently been deprecated in favor of NIOLock.

Changes

All Locks have been renamed to NIOLock.
Application now has two new properties named nioLocks & nioSync, and locks & sync have been marked as deprecated.

Feel free to suggest better names for nioLocks and nioSync.

0xTim
0xTim approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks!

@0xTim 0xTim merged commit 43fc875 into vapor:main Oct 13, 2022
@VaporBot
Copy link
Contributor

These changes are now available in 4.66.1

@ffried
Copy link
Contributor

ffried commented Nov 9, 2022

I've just realized that this is a source breaking change (semver major). The following code would no longer compile (instead of issuing deprecation warnings) with 4.66.1:

import NIOConcurrencyHelpers
import Vapor

struct SomeStruct {
    private enum _LockKey: LockKey {}

    let lock: Lock
    var storage = Array<String>()

    init(app: Application) {
        self.lock = app.locks.lock(for: _LockKey.self) // error: Cannot assign value of type 'NIOLock' to type 'Lock'
    }
}

I'm not sure how often this happens, though.

@0xTim
Copy link
Member

0xTim commented Nov 15, 2022

Ah that's a good point. Unfortunately we've now released so there's not a huge amount to do. I'm surprised this didn't get picked up by the API Checker

@gwynne
Copy link
Member

gwynne commented Nov 15, 2022

It didn't get picked up because we didn't configure the API breakage check to run for this repo until two weeks later 🤦‍♀️

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 15, 2022

In my defense, i did realize this, and that was why i sound surprised in one of the comments above 😶

@ffried
Copy link
Contributor

ffried commented Nov 15, 2022 •
edited

It didn't get picked up because we didn't configure the API breakage check to run for this repo until two weeks later 🤦‍♀️

Perfect timing... 😂

In my defense, i did realize this, and that was why i sound surprised in one of the comments above 😶

I'm not blaming anyone here. Stuff like this happens. 😉
My intention here was to simply document this. It only affected internal playground-code of mine and also fixing it is rather easy. No big deal IMHO.

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 15, 2022

No worries, it didn't sound like blaming 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xTim 0xTim 0xTim approved these changes

Assignees
No one assigned
Labels
semver-patch Internal changes only
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MahdiBM @0xTim @VaporBot @ffried @gwynne