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

NIOTSEventLoops don't participate in MTEventLoopGroup.currentEventLoop #86

Open
gwynne opened this issue May 12, 2020 · 2 comments
Open

Comments

@gwynne
Copy link
Contributor

gwynne commented May 12, 2020

There are two problems with EventLoopGroup as currently concretely implemented:

  1. MultiThreadedEventLoopGroup.threadSpecificEventLoop is a ThreadSpecificVariable, which has unpredictable behavior when used with dispatch queues (which have their own mechanism for the purpose).
  2. There is no way for the generic EventLoopGroup protocol to provide an API for expressing a currentEventLoop concept without making that API public.

This has some unfortunate results when any alternate EventLoopGroup - most especially NIOTSEventLoopGroup is in use:

  • Invoking EventLoopGroup.syncShutdownGracefully() while still on an event loop will fail to diagnose the problem via precondition as it normally would.
  • Misuse of EventLoopFuture.wait() while executing on any event loop will incorrectly report the "current" event loop as nil. In addition, the misuse will only be correctly detected when it takes on the future's own event loop, and even then will suffer even further from NIOTSEventLoop.inEventLoop's "false negative" problem.
  • NIOPipeBootstrap.withPipes() similarly fails to diagnose an attempt to bootstrap while on an event loop.

To make matters even more confusing, it's not clear that NIOTSEventLoop could provide a satisfactory implementation of an API for the appropriate check in the first place, given the aforementioned problems experienced by NIOTSEventLoop.inEventLoop. (See https://github.com/apple/swift-nio-transport-services/blob/master/Sources/NIOTransportServices/NIOTSEventLoop.swift#L84-L94 for details on that issue.)

While all of these uses of currentEventLoop are limited to expressing precondition()s (which is exactly as it should be), and thus these failures are not critical, the lack of these diagnostics tends to make debugging more difficult and delays the detection of such issues until after the problem has become worse. This is not a satisfactory state of affairs, given the recommendation that NIOTSEventLoopGroup be preferred over MultiThreadedEventLoopGroup whenever available - users are left with a recommended default which suffers from (admittedly minor) reduced usability.

It is also possible for other alternative EventLoopGroup implementations to exist, such as a threading model specialized for Windows support, or just for the sheer ridiculousness of it, an EventLoop based on Mach threads (yikes). This suggests that the problem would ideally be solved in a fully generic fashion, rather than providing some form of special-case behavior for NIOTS (if that were possible to begin with).

I have not yet come up with any kind of solution which would not require one of:

  • Exposing the currentEventLoop as public API (which is obviously undesirable; it would only be misused in the same way dispatch_get_current_queue() historically has been)
  • Providing some form of technically public but definitionally private API (as the stdlib does with underscored functions which do not appear in generated interfaces but are nonetheless usable if their names are known)
  • Making NIO nominally aware of NIOTransportServices, which would be a massive layering violation and probably impossible anyway given the obvious circular dependency problem. Upwards dependencies are the bane of sensible build systems, ask anyone who's ever complained that they can't switch away from PBXBuild in their legacy Xcode project 🙃.

I would love to discuss alternatives, if anyone has any thoughts.

@Lukasa
Copy link
Collaborator

Lukasa commented May 12, 2020

Exposing the currentEventLoop as public API (which is obviously undesirable; it would only be misused in the same way dispatch_get_current_queue() historically has been)

Stating this more strongly: I don't believe this is possible, irregardless of whether it's desirable.

I think a first step is: do we believe this problem has a generic solution at all? It's my understanding that in the absence of any context-propagation functionality within Dispatch it is simply not possible to implement a correct version of the "what event loop am I on" function with NIOTS.

Am I missing something?

@gwynne
Copy link
Contributor Author

gwynne commented May 13, 2020

No, you're definitely correct. But it's possible there will eventually be API in Dispatch that allows for it (miracles do happen! 😆), and even if not, it doesn't negate that EventLoopGroup in the generic sense should have a more extensible abstraction for these particular kinds of preconditions. This was one of the intended eventual (if fringe) benefits of adding the ability to specify a failure message to EventLoop's precondition[Not]InEventLoop() methods, except the limitations of Dispatch forced me to withdraw that part of the pull request.

I absolutely do not believe that currentEventLoop should be exposed as public API, or really even as private API. But I do think there should exist a well-named, purpose-constrained abstraction that permits expressing the assertions that it's currently used for, in the same way that Dispatch provides the dispatch_assert_queue*() assertions without the need for dispatch_get_current_queue().

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

No branches or pull requests

2 participants