vapor / vapor

πŸ’§ A server-side Swift HTTP web framework.
https://vapor.codes
MIT License
24.57k stars 1.45k forks source link

Extend Vapor `EventLoop`s with `ThreadSpecificVariable` dictionary #1739

Closed grundoon closed 4 years ago

grundoon commented 6 years ago

Filing for discussion.

Motivation is primarily to allow a standard and straightforward method of creating ThreadSpecificVariables for use on app.eventLoop (rather than have to modify each app separately).

For example, scheduling a repeating task via the main eventLoop often uses a pattern such as

public func boot(_ app: Application) throws {
    runRepeatedTask(app)
}

func runRepeatedTask(_ app: Application) {
    _ = app.eventLoop.scheduleTask(in: .minutes(5)) { () -> Void in
        _ = app.withNewConnection(to: .psql) { db -> Future<Void> in
            // perform query(on: db) here
        }
        runRepeatedTask(app)
    }
}

Which, while simple, encourages discarding the Scheduled<T> result because there's no obvious place to store it without modifying the stock Vapor Application itself (or pushing that struct into persistent storage, ick). Without storage, cancelling the task requires additional logic to determine whether the recursion should be executed, and regardless will only take effect after the current scheduled task has completed, at some future (in:) point.

Adding a ThreadSpecificVariable of type Dictionary<String, Any> = [:] (ob bikeshedding: say, named userDict here) would allow for scenarios such as

func runRepeatedTask(_ app: Application) {
    app.eventLoop.userDict["purgeOldTokens"]: Scheduled = app.eventLoop.scheduleTask(in: .minutes(60)) { () -> Void in
        _ = app.withNewConnection(to: psql) { db -> Future<Void> in
            // perform query(on: db) here
        }
        runRepeatedTask(app)
    }
}

// elsewhere
if let purge = app.eventLoop.userDict["purgeOldTokens"] {
    purge.cancel()
    app.eventLoop.userDict["purgeOldTokens"] = nil
}

Other considerations:

tanner0101 commented 6 years ago

What about just a normal userInfo style dictionary? The app is only intended to be used on its event loop (app.eventLoop). So there shouldn't be a need to make the dictionary thread-safe.

Besides that small thing, I like this idea a lot πŸ‘

MrLotU commented 5 years ago

Think we should close this in favor of #1822

grundoon commented 5 years ago

Part of the original thinking here was that the RFC1123 date for the header was being unnecessarily potentially recalculated on every Response, and could instead be cached upstream on the event loop itself. I don't know that #1822 addresses this type of use case.

tanner0101 commented 5 years ago

On master branch, the Date header is now calculated once per event loop per second using a scheduled task. Each response now just reads whatever the current value is: https://github.com/vapor/http/blob/master/Sources/HTTPKit/Utilities/RFC1123.swift#L70

grundoon commented 5 years ago

Veering off to the side for a moment, should that scheduleRepeatedTask not have a finer-grained initialDelay value in order to closely align it with an actual second flip-over boundary?

tanner0101 commented 5 years ago

That would be a nice feature to add πŸ‘

grundoon commented 5 years ago

Sorry, don't have 5.0 in this box's toolchain so can't really play today, but are you good with something like

    let fracSeconds = 1.0 - Date().timeIntervalSince1970.truncatingRemainder(dividingBy: 1)
    let msToNextSecond = Int(fracSeconds * 1000) + 1
    eventLoop.scheduleRepeatedTask(initialDelay: .milliseconds(msToNextSecond), delay: .seconds(1)) { task in
        new.updateTimestamp()
    }

or should we be looking more along the lines of using clock_gettime?

tanner0101 commented 5 years ago

@grundoon yeah that looks good to me. Are you gonna submit a PR?

grundoon commented 5 years ago

Well, if this would ever finish I could maybe get to actually testing... 🀣

grundoon:http grundoon$ swift --version
Apple Swift version 5.0 (swiftlang-1001.0.60.3 clang-1001.0.37.8)
Target: x86_64-apple-darwin18.2.0
ABI version: 0.7
grundoon:http grundoon$ swift package update
Fetching https://github.com/apple/swift-nio.git
Fetching https://github.com/tanner0101/swift-nio-ssl.git
β–ˆ
tanner0101 commented 4 years ago

Track here: https://github.com/vapor/async-kit/issues/8