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

Consider converting lowest-level I/O primitives from global functions into methods #475

Open
njsmith opened this issue Mar 18, 2018 · 3 comments

Comments

@njsmith
Copy link
Member

njsmith commented Mar 18, 2018

This issue is about Trio's handful of primitive I/O functions: wait_{readable,writable}, wait_socket_{readable,writable}, and a few more exotic ones like wait_kevent, wait_overlapped, etc.

Currently, the public API for these is global functions in trio.hazmat:

trio.hazmat.wait_readable(fd)

I'm wondering if we should change it so that they're methods on a global object you can fetch:

trio.hazmat.current_io_manager().wait_readable(fd)

Implementation-wise, this would be a pretty simple refactoring, because currently the wait_readable function is implemented like:

@enable_ki_protection
def wait_readable(*args, **kwargs):
    try:
        meth = GLOBAL_RUN_CONTEXT.io_manager.wait_readable
    except AttributeError:
        raise RuntimeError("must be called from async context") from None
    return meth(*args, **kwargs)

So in this approach we'd instead have:

def current_io_manager():
    return GLOBAL_RUN_CONTEXT.io_manager

and that's basically it. Though we would need to go through and add explicit @enable_ki_protection decorators to all the methods, since currently they're inheriting it from the wrapper. And make sure that all the private methods are underscored or something. But it's fundamentally not that big a change.

The bigger implication is for the public API:

  • Currently you can check which primitives are available by doing checks like hasattr(trio.hazmat, "wait_readable"), and this is a static fact for the life of the process. For example, you can do it at import time. Now this wouldn't be true; you'd have to do the checks on the I/O manager object, and you couldn't do it until you were inside an async context.

  • And, of course, this is the whole point: it would make it possible for us to delay deciding which I/O manager we were using until runtime. I'm still not excited about the idea of adding a Qt-backed I/O manager (see Using trio with Qt's event loop? #399 for more discussion), but at least it would be structurally possible if we decided to do it at some point in the future.

@njsmith
Copy link
Member Author

njsmith commented Dec 15, 2018

Concretely this probably involves:

  • Defining a new, public, current_io_manager function, like above
  • Going through all the IOManager classes, and adding underscores to all the private methods
  • Going through all the IOManager classes, and removing @_public from the public methods, and replacing it with @enable_ki_protection
  • Adding compatibility shims like def wait_socket_readable(...): return current_io_manager().wait_socket_readable(...) in trio/hazmat.py, and marking these all with @deprecated
  • Going through everywhere we use the I/O functions like wait_socket_readable in Trio, and switching them to use current_io_manager() instead of the old global functions.

@njsmith
Copy link
Member Author

njsmith commented Dec 15, 2018

I've been saying we should do this for #542, but actually I'm still feeling a bit indecisive about whether this is a good idea. There are two independent motivations, which is normally a pretty strong argument. But how strong are they really?

Basically the issue is that right now, the set of supported low-level operations is determined at import time.

Problem 1: If we want to do static analysis, like type-checking and linting (#542), then import time is too late: analysis tools can't tell which functions are actually available, or what their signatures are, because it's hidden behind dynamic code. They want to see the actual functions/methods directly in the source code on disk, without executing anything.

The current_io_manager() trick doesn't entirely solve this – instead of having a bunch of module-level functions that appear and disappear based on the underlying OS, we would instead have a bunch of class-level methods that appear and disappear based on the underlying OS. How do we type current_io_manager() to make mypy happy? We could have it return Union[EpollIOManager, WindowsIOManager, KQueueIOManager], but I suspect that will make it annoying to use, since mypy will insist that you always check which kind you have before calling any methods, even if you're writing Linux-only code, and I'm guessing tab-completion would be ugly. We could have it return IOManager, where that's defined using an if chain that static analysis tools can understand... that's actually tricky though. mypy can understand if statements involving sys.platform. That's not actually how we figure out which IOManager to use, but I think we could kluge it a bit with clauses like if sys.platform == "linux" or hasattr(select, "epoll"), where the first part is for mypy and the latter is for runtime (and mypy may give wrong results on rare platforms that have epoll, like illumos, but oh well). OTOH, this doesn't help other tools like pylint or pycharm that don't implement the exact same tricks as mypy. I doubt jedi could provide completion for io_manager = current_io_manager(); io_manager.<tab>.

What's the alternative? We could keep the current module-level functions, make them statically visible via code generation (which we already need to do for the runner methods), and wrap the statically generated global functions in if sys.platform ... blocks. I.e., basically it's the same situation as for IOManager. Or, we could define them unconditionally, and have them raise NotImplementedError or similar if you try to call them on the wrong platform. This is a little unidiomatic, since it breaks hasattr checks, but might be fine? And it might actually make life easier for mypy users, since if you have a portable code base, it probably will contain calls to functions that aren't available on the platform where you happened to run mypy, and that's OK.

Problem 2: Import-time selection is also problematic in case we ever want to support alternative I/O backends in the future (e.g. Qt), because it happens too early: at import time we don't know whether someone is going to do trio.run(..., backend="epoll") or trio.run(..., backend="qt").

I'm still hopeful that we can support Qt etc. without actually supporting multiple I/O backends (see #399 for discussion of alternatives). So maybe this will never come up. OTOH I'll feel very silly if like 5 years from now we discover some new reason why this is super important, but we're trapped by old decisions.

If we switch to defining the method wrappers unconditionally, then this problem goes away... if you try to call like, a kqueue-specific method on a platform that normally has kqueue, but that doesn't becuase we're using a weird I/O backend, then it just means raises NotImplementedError, same as any platform that doesn't have kqueue available.

@njsmith
Copy link
Member Author

njsmith commented Dec 15, 2018

I guess it would also be fine to make the basic function availability be determined by platform, and then on top of that have functions that error out if you try to call them on the wrong backend. The operations that e.g. Qt can support is going to be a subset of all the operations the platform can support.

I'm also wondering if we should drop the distinction between wait_readable/wait_writable/notify_fd_close vs. wait_socket_readable/wait_socket_writable/notify_socket_close. The alternative of having a single set that takes fds on Unix and HANDLEs on Windows is gross, but I don't know that what we're doing is actually improving anything, especially since we started accepting raw integers in wait_socket_* (#610).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants