Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions Package.resolved

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ let package = Package(
.library(name: "S3Store", targets: ["S3Store"]),
],
dependencies: [
.package(url: "https://github.com/0xLeif/Cache.git", from: "2.1.0"),
.package(url: "https://github.com/JohnSundell/Ink.git", from: "0.5.1"),
.package(url: "https://github.com/swift-server/swift-prometheus.git", from: "1.0.0"),
.package(url: "https://github.com/SwiftPackageIndex/Plot.git", branch: "main"),
Expand Down Expand Up @@ -63,7 +62,6 @@ let package = Package(
.product(name: "SPIManifest", package: "SPIManifest"),
.product(name: "SemanticVersion", package: "SemanticVersion"),
.product(name: "SwiftSoup", package: "SwiftSoup"),
.product(name: "Cache", package: "cache"),
.product(name: "CanonicalPackageURL", package: "CanonicalPackageURL"),
.product(name: "CustomDump", package: "swift-custom-dump"),
.product(name: "Dependencies", package: "swift-dependencies"),
Expand Down
32 changes: 0 additions & 32 deletions Sources/App/Core/CurrentReferenceCache.swift

This file was deleted.

86 changes: 86 additions & 0 deletions Sources/App/Core/Dependencies/CurrentReferenceCacheClient.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Copyright Dave Verwer, Sven A. Schmidt, and other contributors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import Dependencies
import DependenciesMacros


@DependencyClient
struct CurrentReferenceCacheClient {
var set: @Sendable (_ owner: String, _ repository: String, _ reference: String?) async -> Void
var get: @Sendable (_ owner: String, _ repository: String) async -> String?
}


extension CurrentReferenceCacheClient: DependencyKey {
static let timeToLive: Duration = .seconds(5*60)

static var liveValue: CurrentReferenceCacheClient {
.init(
set: { owner, repository, reference async in
@Dependency(\.redis) var redis
await redis.set(key: getKey(owner: owner, repository: repository),
value: reference,
expiresIn: timeToLive)
Copy link
Member Author

Choose a reason for hiding this comment

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

@heckj , this is how you'd interface with Redis once this lands. We wouldn't be using app.redis at all but rather the private Redis actor via the RedisClient dependency, replacing your PR #3580.

The only thing this doesn't cover is setup for local use of Redis, i.e. during development. It should be easy to add though, either by making the hostname configurable or just setting a redis alias in /etc/hosts on the local machine. The latter is certainly easiest and fastest to get going.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correction, I just realised I misread your PR #3580. I was focused on the part

-        app.redis.configuration = try RedisConfiguration(hostname: "redis")
+        if let redisHost = Environment.get("REDIS_HOST") {
+            app.redis.configuration = try RedisConfiguration(hostname: redisHost)
+        } else {
+            app.logger.warning("REDIS_HOST not set, caching disabled.")
+        }

but really the point of it is to add a bringup for a local Redis and the hostname config.

},
get: { owner, repository in
@Dependency(\.redis) var redis
return await redis.get(key: getKey(owner: owner, repository: repository))
}
)
}

static func getKey(owner: String, repository: String) -> String {
"\(owner)/\(repository)".lowercased()
}
}


extension CurrentReferenceCacheClient: TestDependencyKey {
static var testValue: Self { Self() }
}


extension DependencyValues {
var currentReferenceCache: CurrentReferenceCacheClient {
get { self[CurrentReferenceCacheClient.self] }
set { self[CurrentReferenceCacheClient.self] = newValue }
}
}


#if DEBUG
extension CurrentReferenceCacheClient {
static var disabled: Self {
.init(set: { _, _, _ in }, get: { _, _ in nil })
}

static var inMemory: Self {
.init(
set: { owner, repository, reference in
_cache.withValue {
$0[getKey(owner: owner, repository: repository)] = reference
}
},
get: { owner, repository in
_cache.withValue {
$0[getKey(owner: owner, repository: repository)]
}
}
)
}

nonisolated(unsafe) static var _cache = LockIsolated<[String: String]>([:])
}
#endif
2 changes: 0 additions & 2 deletions Sources/App/Core/Dependencies/EnvironmentClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ struct EnvironmentClient {
var collectionSigningCertificateChain: @Sendable () -> [URL] = { XCTFail("collectionSigningCertificateChain"); return [] }
var collectionSigningPrivateKey: @Sendable () -> Data?
var current: @Sendable () -> Environment = { XCTFail("current"); return .development }
var currentReferenceCache: @Sendable () -> CurrentReferenceCache?
var dbId: @Sendable () -> String?
var mastodonCredentials: @Sendable () -> Mastodon.Credentials?
var random: @Sendable (_ range: ClosedRange<Double>) -> Double = { XCTFail("random"); return Double.random(in: $0) }
Expand Down Expand Up @@ -102,7 +101,6 @@ extension EnvironmentClient: DependencyKey {
Environment.get("COLLECTION_SIGNING_PRIVATE_KEY").map { Data($0.utf8) }
},
current: { (try? Environment.detect()) ?? .development },
currentReferenceCache: { .live },
dbId: { Environment.get("DATABASE_ID") },
mastodonCredentials: {
Environment.get("MASTODON_ACCESS_TOKEN")
Expand Down
127 changes: 127 additions & 0 deletions Sources/App/Core/Dependencies/RedisClient.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
// Copyright Dave Verwer, Sven A. Schmidt, and other contributors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import NIOCore
@preconcurrency import RediStack
import Dependencies
import DependenciesMacros


@DependencyClient
struct RedisClient {
var set: @Sendable (_ key: String, _ value: String?, Duration?) async -> Void
var get: @Sendable (_ key: String) async -> String?
}


extension RedisClient {
func set(key: String, value: String?, expiresIn: Duration? = nil) async {
await set(key: key, value: value, expiresIn)
}
}


extension RedisClient: DependencyKey {
static var liveValue: RedisClient {
.init(
set: { key, value, expiresIn in
await Redis.shared?.set(key: key, value: value, expiresIn: expiresIn)
},
get: { key in await Redis.shared?.get(key: key) }
)
}
}


extension RedisClient: TestDependencyKey {
static var testValue: Self { Self() }
}


extension DependencyValues {
var redis: RedisClient {
get { self[RedisClient.self] }
set { self[RedisClient.self] = newValue }
}
}


#if DEBUG
extension RedisClient {
static var disabled: Self {
.init(set: { _, _, _ in }, get: { _ in nil })
}
}
#endif


private actor Redis {
var client: RediStack.RedisClient
static private var task: Task<Redis?, Never>?

static var shared: Redis? {
get async {
if let task {
return await task.value
}
let task = Task<Redis?, Never> {
var attemptsLeft = maxConnectionAttempts
while attemptsLeft > 0 {
do {
return try await Redis()
} catch {
attemptsLeft -= 1
Current.logger().warning("Redis connection failed, \(attemptsLeft) attempts left. Error: \(error)")
try? await Task.sleep(for: .milliseconds(500))
}
}
return nil
}
self.task = task
return await task.value
}
}

private init() async throws {
let connection = RedisConnection.make(
configuration: try .init(hostname: Redis.hostname),
boundEventLoop: NIOSingletons.posixEventLoopGroup.any()
)
self.client = try await connection.get()
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Please give this whole actor Redis a thorough review, in particular how it's setting up the shared singleton with retries and the async throws initialiser that's using the @preconcurrency imported RedisConnection.

In particular, @preconcurrency is required, because RedisConnection isn't Sendable. I think this is correctly set up but good to have a proper review of this.

Unfortunately, there's not much I can see us do around this in terms of testing other than verifying that writing to and reading from Redis works on dev (which it does).


// This hostname has to match the redir service name in app.yml.
static let hostname = "redis"
Copy link
Member Author

Choose a reason for hiding this comment

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

We could make this configurable but there's not much point as it's not really dependent on anything, just a static name defined in app.yml.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was the only bit that I thought we might want to tweak and make into an environment variable, primarily so that a purely local development setup could reference LOCALHOST:6379 and had redis running in a container.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've simply set 127.0.0.1 redis in my /etc/hosts locally but of course making in configurable is better.

static let maxConnectionAttempts = 3

func set(key: String, value: String?, expiresIn: Duration?) async -> Void {
if let value {
let buffer = ByteBuffer(string: value)
let value = RESPValue.bulkString(buffer)
if let expiresIn {
let ttl = Int(expiresIn.components.seconds)
try? await client.setex(.init(key), to: value, expirationInSeconds: ttl).get()
} else {
try? await client.set(.init(key), to: value).get()
}
} else {
_ = try? await client.delete([.init(key)]).get()
}
}

func get(key: String) async -> String? {
return try? await client.get(.init(key)).map(\.string).get()
}
}

8 changes: 0 additions & 8 deletions Sources/App/configure.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

import Fluent
import FluentPostgresDriver
import Redis
import Vapor


Expand Down Expand Up @@ -78,13 +77,6 @@ public func configure(_ app: Application) async throws -> String {
sqlLogLevel: .debug),
as: .psql)

// Setup Redis connection
do {
app.redis.configuration = try RedisConfiguration(hostname: "redis")
} catch {
app.logger.warning("Failed to configure Redis, caching disabled. Error: \(error)")
}

do { // Migration 001 - schema 1.0
app.migrations.add(CreatePackage())
app.migrations.add(CreateRepository())
Expand Down
5 changes: 3 additions & 2 deletions Sources/App/routes+documentation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,15 @@ extension Request {

let docVersion = try await { () -> DocVersion in
if reference == String.current {
if let ref = environment.currentReferenceCache()?[owner: owner, repository: repository] {
@Dependency(\.currentReferenceCache) var currentReferenceCache
if let ref = await currentReferenceCache.get(owner: owner, repository: repository) {
return .current(referencing: ref)
}

guard let params = try await DocumentationTarget.query(on: db, owner: owner, repository: repository)?.internal
else { throw Abort(.notFound) }

environment.currentReferenceCache()?[owner: owner, repository: repository] = "\(params.docVersion)"
await currentReferenceCache.set(owner: owner, repository: repository, reference: "\(params.docVersion)")
return .current(referencing: "\(params.docVersion)")
} else {
return .reference(reference)
Expand Down
Loading
Loading