search

jocosocial / swiftarr

The V3 Twitarr server and templated web interface.
MIT License
17 stars 9 forks source link

Vapor Storage Threading #206

Closed cohoe closed 7 months ago

cohoe commented 1 year ago

We got burned by https://github.com/vapor/vapor/issues/2330 again. While we fixed the websocket storage in https://github.com/jocosocial/swiftarr/issues/53, we didn't consider that under the hood Vapor also uses this for Redis/Postgres connection storage (among other things). Chall was able to hack together a custom version of the Vapor framework to get around this problem on boat. There is an open PR on Vapor that would address this: https://github.com/vapor/vapor/pull/2873

When we start getting closer to 2024 we should either:

hendricksond commented 1 year ago

https://github.com/vapor/vapor/pull/2873 was closed in favor of https://github.com/vapor/vapor/pull/3000. Monitor the new PR.

cohoe commented 9 months ago

https://github.com/vapor/vapor/pull/3000 was closed in favor of other PRs, such as https://github.com/vapor/vapor/pull/3082 and https://github.com/vapor/vapor/pull/3093. The latter of which indicates that the move to Sendable is complete. We'd need to update to Vapor >= 4.86.0.

cohoe commented 9 months ago

Package.resolved already has Vapor up to 4.89.0 from when Chall last did an update not that long ago. Based on https://forums.swift.org/t/concurrency-checking-in-swift-packages-unsafeflags/61135/3 we can add the following block to the Package.swift if we want to get compilation warnings about potential concurrency issues:

swiftSettings: [
    // This one is the most strict
    // .unsafeFlags(["-Xfrontend", "-strict-concurrency=complete"])
    .unsafeFlags(["-Xfrontend", "-warn-concurrency"])
]

Which indeed spits out a bunch but mostly seems to indicate that we're supposed to mark our API handler functions with @Sendable (for example: @Sendable func openHandler(_ req: Request) async throws -> FezListData {...}.

I don't know if marking all of those is necessary or if it just generates a warning until you do.

cohoe commented 7 months ago

We haven't seen this yet. Closing now, so that I can re-open with much sadness if it comes up day one again.