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.

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

Add ThreadOwnershipModel enum #810

Closed
wants to merge 1 commit into from
Closed

Conversation

Mordil
Copy link
Contributor

@Mordil Mordil commented Feb 8, 2019

Motiviation:

As discussed in the pitch for NIORedis and NIOPostgres (https://forums.swift.org/t/swiftnio-redis-client/19325 and https://forums.swift.org/t/pitch-swiftnio-based-postgresql-client/18020),
an idea has been floated of providing a ThreadOwnership model to provide capabilities of these libraries to be used "out of the box" without having to think about setting up and maintaining
EventLoopGroups by end users.

It might be useful to have this type be defined in NIO itself, so as framework and library authors can share this type and reference it in whatever features they may want to build.

An example of use can be found in NIORedis at https://github.com/Mordil/nio-redis/blob/e168b6c65eb471e186fd43623af93a4902564756/Sources/NIORedis/RedisDriver.swift

Modifications:

Added a ThreadOwnershipModel public enum with two cases: internal and external

Results:

A new enum type that implementers can use to define how their types can be used to manage NIO threading resources.

@swift-nio-bot
Copy link

Can one of the admins verify this patch?

3 similar comments
@swift-nio-bot
Copy link

Can one of the admins verify this patch?

@swift-nio-bot
Copy link

Can one of the admins verify this patch?

@swift-nio-bot
Copy link

Can one of the admins verify this patch?

Motiviation:

As discussed in the pitch for NIORedis and NIOPostgres (https://forums.swift.org/t/swiftnio-redis-client/19325 and https://forums.swift.org/t/pitch-swiftnio-based-postgresql-client/18020),
an idea has been floated of providing a `ThreadOwnership` model to provide capabilities of these libraries to be used "out of the box" without having to think about setting up and maintaining
EventLoopGroups by end users.

It might be useful to have this type be defined in NIO itself, so as framework and library authors can share this type and reference it in whatever features they may want to build.

An example of use can be found in NIORedis at https://github.com/Mordil/nio-redis/blob/e168b6c65eb471e186fd43623af93a4902564756/Sources/NIORedis/RedisDriver.swift

Modifications:

Added a `ThreadOwnershipModel` public enum with two cases: `internal` and `external`

Results:

A new enum type that implementers can use to define how their types can be used to manage NIO threading resources.
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks for opening this and for the suggestion!

However, I'm a -1 on this idea. While I understand the desire to standardise around the interface for this kind of thing, I think this is a substantial oversimplification of the problem that, while suitable for NIORedis and NIOPostgres in the current moment, is not going to age well.

In particular, the "thread ownership model" here suggests that all event loop groups are equal, which they are not. In particular, the EventLoopGroup existential provided here is not actually useful, as it must always be conditionally downcast to a usable data type (e.g. MultiThreadedEventLoopGroup).

I think any attempt to standardise something like this will need to first address the proliferation of Bootstraps. This will likely require defining one or more protocols that indicate the baseline of what a bootstrap actually supports, along with providing associated types that indicate what type of event loop group is required for a given bootstrap.

With sufficient care we should be able to design a model that will allow libraries to express much more clearly what kinds of event loop groups they support or require, as well as encompass some notion of a thread ownership model.

For the moment, though, I'm fine with this enum being repeated in a few different modules: it's only a few lines of code.

@Mordil
Copy link
Contributor Author

Mordil commented Feb 8, 2019

@Lukasa That's definitely reasonable - especially from NIO's perspective on the problem and solution.

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.

3 participants