Added detection for Twisted

[agronholm]

No description provided.

[codecov]

Codecov Report

Merging #10 (b1dbc3c) into master (0cfdab8) will increase coverage by 1.02%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   96.51%   97.54%   +1.02%     
==========================================
  Files           4        4              
  Lines          86      122      +36     
  Branches       17       22       +5     
==========================================
+ Hits           83      119      +36     
  Misses          2        2              
  Partials        1        1              

Files	Coverage Δ
sniffio/_impl.py	92.10% <100.00%> (+1.19%)	⬆️
sniffio/_tests/test_sniffio.py	100.00% <100.00%> (ø)

[njsmith]

This looks pretty fragile... What if someone is using a reactor besides the global one so the reactor.running check is wrong, or using a third party reactor that doesn't live in twisted.internet, or is using aioreactor and we need to detect whether it's in "twisted context" or "asyncio context"?

I'd be more comfortable if we could get some feedback from the twisted devs. I know @hawkowl is working on adding context var support, which would make all this much easier...

[hawkowl]

In a real application right now, there's too much code that exists that relies on the global reactor for it to not be installed. Removing that is the plan, but even when contextvars support lands, it'll be optional for Twisted (at least until 3.7 becomes the earliest we support).

BTW, twisted.internet.reactor is a reference to the currently installed reactor that is running globally, not any particular concrete implementation. If you're using an alternate reactor, you're supposed to call installReactor with it as an argument to make the reactor you've instantiated be installed "globally". .running is also part of the interface and an expected public API for any reactor (including asyncioreactor) to implement, IIRC.

[hawkowl]

WRT the checking if it's running in the same thread -- Twisted's reactor is "threadsafe" in that IReactorThreads supports callFromThread to allow calls from other threads. Twisted does not currently support multiple reactors in the same process (since it's pointless with the GIL and you run into stampeding herd problems with epoll that means it's just not worth it)

[njsmith]

@hawkowl There's a PyPI backport of contextvars that works fine back to 3.5 at least.

The goal of this project is to make it so you can do await http_client.get(...) and http_client can use sniffio to automatically figure out whether it's supposed to use its twisted backend or asyncio backend or what. So the question is which coroutine runner is running. It would be pretty easy for Twisted to export this information directly: there's a ContextVar that you would set to "twisted" when inside inlineCallbacks and not otherwise. Obviously this is easier though if Twisted knows about contextvars :-). Details: https://sniffio.readthedocs.io/en/latest/#adding-support-to-a-new-async-library

The PR here uses stack introspection to see if the global reactor is running + it's inside twisted/internet/base.py:run, and if so then it guesses that we must be using twisted. I guess if we don't care about supporting non-global reactors, then the first part is OK. The concern with the second part is that third-party reactors might have their own run methods instead of using the built-in one. I don't know how likely that is in practice.

The really big practical issue though is that if you're using asyncioreactor, then you'll have some code inside the same thread that's running inside the asyncio coroutine runner (asyncio.Task), and some that's running inside the twisted coroutine runner (inlineCallbacks), and our http_client.get needs to do different things depending on which case it finds itself in. So I don't think any mechanism based on detecting the state of the global reactor can possibly work; the determination needs to be more local.

So I guess doing stack introspection to check whether we're inside the inlineCallbacks loop would be a more workable heuristic? Specifically twisted.internet.defer._inlineCallbacks. But it would be much cleaner and nicer if Twisted could provide this information instead of making us use a heuristic.

[agronholm]

WRT the checking if it's running in the same thread -- Twisted's reactor is "threadsafe" in that IReactorThreads supports callFromThread to allow calls from other threads.

The stack introspection approach was the only thing I could think of that would work. My first attempt involved callFromThread(), but it seems like it doesn't return the return value of the callable, so how am I supposed to wait for the call to complete?

In a real application right now, there's too much code that exists that relies on the global reactor for it to not be installed. Removing that is the plan, but even when contextvars support lands, it'll be optional for Twisted

Is the plan going to be to have threadlocal reactors then, or just passing a reactor instance around? I really hope it's not the latter. Asyncio at least is doing away with the pattern of passing around the event loop object, and that is an improvement IMHO.

The really big practical issue though is that if you're using asyncioreactor, then you'll have some code inside the same thread that's running inside the asyncio coroutine runner (asyncio.Task), and some that's running inside the twisted coroutine runner (inlineCallbacks), and our http_client.get needs to do different things depending on which case it finds itself in. So I don't think any mechanism based on detecting the state of the global reactor can possibly work; the determination needs to be more local.

My thinking was along the lines of using the asyncio code path if Twisted is using the asyncio reactor. This should work, yes? I don't have enough Twisted experience to be sure though.

[njsmith]

My understanding is that the asyncio and twisted coroutine runners are currently incompatible, because of using different yield protocols. (asyncio yields Futures, twisted yields Deferreds, and Future and Deferred are incompatible with each other.) So if you're inside a twisted-flavored async function, you have to use twisted primitives, not asyncio primitives, even with the asyncioreactor.

I wouldn't be too shocked though if it turns out I misunderstood something.

[agronholm]

My understanding if you need to use asyncio coroutines or futures, you need to wrap them with Deferred.fromFuture() (and for coroutines, first starting asyncio Tasks from them using asyncio.ensureFuture()). But once you're inside an asyncio coroutine, you can use asyncio code as you please.

[agronholm]

This of course does mean that you can't just use asyncio code path as-is, but it could possibly be wrapped appropriately for Twisted. I'd be happy to make a native Twisted back-end, if that is deemed possible.

[agronholm]

BTW, what to do about the 3.8 test failure? It's just a consequence of Twisted using the deprecated cmp attribute in attrs and the CI script turning warnings into errors. I personally don't see a point in doing that, but then there are a lot of other things I don't agree on.

[njsmith]

My understanding if you need to use asyncio coroutines or futures, you need to wrap them with Deferred.fromFuture() (and for coroutines, first starting asyncio Tasks from them using asyncio.ensureFuture()). But once you're inside an asyncio coroutine, you can use asyncio code as you please.

Sure, but the whole point of sniffio is to figure out whether you're in an asyncio coroutine or a twisted coroutine.

BTW, what to do about the 3.8 test failure? It's just a consequence of Twisted using the deprecated cmp attribute in attrs and the CI script turning warnings into errors. I personally don't see a point in doing that, but then there are a lot of other things I don't agree on.

The point is to make sure we notice this kind of issue first, before our users do. In this case, there's nothing for us to do about this warning, so we should just add a filter to ignore that warning. But unfortunately we have to look at it to know that; there's no way to know ahead of time which warnings are spurious.

[njsmith]

The code is way nicer now, but I think the behavior is still fundamentally broken if folks are mixing asyncio+twisted code in the same event loop, which seems like a very common use case, and increasingly common over time :-(. At the least we need a test that checks that different combinations of asFuture/fromFuture/ensureDeferred work properly.

[agronholm]

I'm not sure I understand. In what circumstances do you see that the check would return the wrong result? Can you give me a pseudocode example?

[njsmith]

async def aio_flavored():
    assert sniffio.current_async_library() == "asyncio"

async def twisted_flavored():
    assert sniffio.current_async_library() == "twisted"
    await Deferred.fromFuture(asyncio.ensure_future(aio_flavored())

react(lambda _: ensureDeferred(twisted_flavored()))

[agronholm]

I just checked, and from within that Twisted coroutine, both reactor.running and isInIoThread() return False. Back to the drawing board I guess.

[njsmith]

Huh. That.... is not the problem I was expecting.

[agronholm]

I took a closer look at this and it appears that Twisted's @inlineCallbacks is running the coroutine to get the first awaitable. Only then is _reactor.run() being called. The code could be refactored to run the callable in a running reactor, but I don't know if the Twisted devs are open to such a change, given their current direction of design which requires the reactor object to be passed around.

[njsmith]

AFAIK @inlineCallbacks doesn't use the reactor at all (the code inside the coroutine is responsible for eventually scheduling callbacks onto the reactor instead). So I'm not sure what you mean about _reactor.run.

...Oh, I guess the issue is that if you're using react(func_defined_using_inlineCallbacks()), then that first coroutine starts executing before the reactor starts running? That sounds plausible. I guess that's another reason why introspecting reactor.running is not a good way to detect whether you're inside inlineCallbacks. So that explains why you're seeing that particular symptom.

In the grand scheme of things though, I don't think this makes any difference, because there were already other reasons why introspecting the reactor didn't work :-).

Basically we want current_async_library to return twisted iff it's run inside @inlineCallbacks, and not otherwise; we don't actually have to care about the reactor at all. The implementation isn't obvious – the gross way requires something involving stack introspection, the good way requires changes to twisted – but the desired behavior is clear.

[agronholm]

I modified the react() function in such a way that it always runs the callable inside the reactor. This fixes our problem (and simplifies the code in the process):

def react(main, argv=(), _reactor=None):
    if _reactor is None:
        from twisted.internet import reactor as _reactor
    codes = [0]

    def runMain():
        try:
            finished = main(_reactor, *argv)
        except BaseException as exc:
            cbFinish(Failure(exc))
        else:
            finished.addBoth(cbFinish)

    def cbFinish(result):
        try:
            _reactor.stop()
        except ReactorNotRunning:
            pass

        if isinstance(result, Failure):
            if result.check(SystemExit) is not None:
                codes[0] = result.value.code
            else:
                log.err(result, "main function encountered error")
                codes[0] = 1

    _reactor.callWhenRunning(runMain)
    _reactor.run()
    sys.exit(codes[0])