Skip to content

Use NIO in Single Threaded Mode #68

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.

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

Merged
merged 6 commits into from
May 12, 2020
Merged

Use NIO in Single Threaded Mode #68

merged 6 commits into from
May 12, 2020

Conversation

fabianfett
Copy link
Member

@fabianfett fabianfett commented May 2, 2020

Use @weissi's awesome pr...
apple/swift-nio#1499

Remains in draft until 2.17 is released.

Don't review this yet. This is just made to compile & run.

@fabianfett fabianfett force-pushed the use-main-as-eventloop branch 2 times, most recently from a1067a6 to a5b6dcc Compare May 4, 2020 11:56
lifecycle.shutdown()
}

_ = lifecycle.start().flatMap {
Copy link
Member Author

@fabianfett fabianfett May 4, 2020

Choose a reason for hiding this comment

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

lifecycle.start() runs in the eventLoop now.

This means that the factory here is executed on the eventLoop.

    // for testing and internal use
    @discardableResult
    internal static func run(configuration: Configuration = .init(), factory: @escaping (EventLoop) throws -> Handler) -> Result<Int, Error> {
        self.run(configuration: configuration, factory: { eventloop -> EventLoopFuture<Handler> in
            do {
                let handler = try factory(eventloop)
                return eventloop.makeSucceededFuture(handler)
            } catch {
                return eventloop.makeFailedFuture(error)
            }
        })
    }

We should probably offload that as well.

@fabianfett fabianfett force-pushed the use-main-as-eventloop branch from a5b6dcc to b7beddd Compare May 4, 2020 12:09
} catch {
return eventloop.makeFailedFuture(error)
let promise = eventloop.makePromise(of: Handler.self)
Lambda.defaultOffloadQueue.async {
Copy link
Member Author

@fabianfett fabianfett May 4, 2020

Choose a reason for hiding this comment

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

Addresses the problem stated here:
https://github.com/swift-server/swift-aws-lambda-runtime/pull/68/files#r419383593

I unsure though if using Lambda.defaultOffloadQueue is a good solution to the problem.

@fabianfett fabianfett force-pushed the use-main-as-eventloop branch from b7beddd to bbd6382 Compare May 6, 2020 15:53
@fabianfett fabianfett force-pushed the use-main-as-eventloop branch 2 times, most recently from ce91afb to 380d8ce Compare May 12, 2020 14:57
@fabianfett fabianfett requested review from tomerd and weissi May 12, 2020 14:57
@fabianfett fabianfett marked this pull request as ready for review May 12, 2020 14:58
@fabianfett fabianfett force-pushed the use-main-as-eventloop branch from 380d8ce to 4cc896e Compare May 12, 2020 14:58
Comment on lines +83 to +93
let promise = eventloop.makePromise(of: Handler.self)
// if we have a callback based handler factory, we offload the creation of the handler
// onto the default offload queue, to ensure that the eventloop is never blocked.
Lambda.defaultOffloadQueue.async {
do {
promise.succeed(try factory(eventloop))
} catch {
promise.fail(error)
}
}
return promise.futureResult
Copy link
Member Author

@fabianfett fabianfett May 12, 2020

Choose a reason for hiding this comment

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

👀 @tomerd @weissi I'm not sure if this is the best solution to the problem?!

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm :)

return lifecycle.start().flatMap {
return lifecycle.shutdownFuture.always { _ in

var r: Result<Int, Error>?
Copy link
Contributor

@tomerd tomerd May 12, 2020

Choose a reason for hiding this comment

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

r -> result

Copy link
Contributor

Choose a reason for hiding this comment

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

also maybe Result<Int, Error>! instead of Result<Int, Error>?

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 have two results here, one within the function scope and one within the .always closure.

signalSource.cancel()
eventLoop.shutdownGracefully { _ in
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do in the MultiThreadedEventLoopGroup.withCurrentThreadAsEventLoop case?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tomerd it makes withCurrentThreadAsEventLoop return

Copy link
Contributor

Choose a reason for hiding this comment

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

@fabianfett actually, you gotta handle the error there :). The right way is

if let error = error {
    preconditionFailure("Failed to shutdown eventloop: \(error)")
}

The reason this is a precondition is because this has to work, unless you have a programmer error (tried to shut it down twice or so).

Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

lgtm, one naming issue and one question

@fabianfett
Copy link
Member Author

@tomerd fixed.

_ = lifecycle.start().flatMap {
lifecycle.shutdownFuture
}
.always { lifecycleResult in
Copy link
Contributor

Choose a reason for hiding this comment

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

actually maybe we can use whenComplete instead of always here so we dont need to do _ =

Copy link
Member Author

Choose a reason for hiding this comment

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

oh sure. fixed!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@fabianfett fabianfett force-pushed the use-main-as-eventloop branch from 9f8f864 to f967460 Compare May 12, 2020 20:25
@tomerd tomerd merged commit ab3ce64 into master May 12, 2020
@tomerd tomerd deleted the use-main-as-eventloop branch May 12, 2020 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants