-
Notifications
You must be signed in to change notification settings - Fork 763
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
a Future constructor is missing the loop
kwarg
#747
Comments
Hmm, yeah that's wrong. It actually should be using If we keep the bare Thanks for the pointers on your unit-test. We're not using pytest in this particular case, but do have a similar test-helper in @petri if you can post your code somewhere that'd be ideal :) (For your use-case, replacing "the" loop globally via |
I was having the exact some problem, but I think the loop needs to be referenced as |
Yes, #748 may be "a" stop-gap solution, but we need some tests for this and also need to figure out the implications of not using the Several options:
|
I have a use-case question, too: does anyone need a loop-per-
|
I thought txaio is only meant to be used to provide an abstraction layer so code can run unmodified both on asyncio and twisted? So why would it be needed in the asyncio-specific package? |
Yes, we can use asyncio-only APIs inside this asyncio-specific package. However, txaio has its own concept of "the" event loop and this stuff bypasses all that -- so needs some consideration. That is, if you set up an event loop in txaio and then run this you'll be sad that your event-loop isn't getting used by these websocket connections. (Also, |
What I think we should do is: remove the ...BUT that removes a currently-existing public API is mostly why I added the needs-discussion tag. |
Explicit pass of loop is de facto for asyncio applications. So another hard option:
|
Well, we can't have But, I guess you're suggesting some sort of intermediate object that gets passed a loop (and in Twisted case I guess it would just have to throw an exception if the loop isn't "the one true reactor"). e.g. something like I still ask, though: is there really a use-case for N loops? Or is "default loop or 1 explicit loop" sufficient? Can you point me to examples of asyncio code that use > 1 loop (and why they do that)? The use-case in this particular issue is for testing, which seems completely sufficient to have just one replacement loop. Another possible solution:
This would, hopefully (and, should have tests!) mean that someone who never passed |
I don't know about the cross-platform compatibility, but the autobahn.asyncio.websocket.WebSocketServerFactory (which is a dedicated class for use with asyncio) is explicitly accepting a loop keyword in its constructor and the usage of the As for an example usage with multiple event loops: I am currently working on a deployment script that can manage multiple services, each running in a separate thread, and each service (possibly) using different main-loop implementations. So this library needs to be able to handle a deployment of (I'm making up an example scenario here) four different services, where one service implements a threaded web-server where the main thread just accepts connections and spawns a new thread for each connection, an FTP service that makes use of an ftp library that operates using calls to select.epoll(), and two other services that use asyncio, but possibly using different helper libraries. The library in this scenario has no clue whatsoever what technologies are being used for each service and the services don't know about each other's implementations, either. This is why each of these service implementations must be able to manage a dedicated event loop in my case. I encountered this bug during the implementation of an autobahn-powered websocket service. |
I agree this class is breaking the implied contract from the BOTH of the above need fixing, IMO. Possibly this means augmenting Is your example scenario covered if "all the Autobahn WebSockets use the custom event loop you gave to |
Not really. The issue for me is that there is a global event loop, which prevents me from running multiple loops in different threads. Actually my requirements are even more restrictive than that: I cannot even work with a thread-local loop, since the loop might receive new tasks from other threads. I'm aware, that these are pretty unusual constraints. |
Here is my view on this:
In principle there seem to be 3 options of what we could do: a) remove a) covers more use cases: it would allow switching the one global event loop, but it does not allow multiple loops concurrently - neither in one and the same, nor in different threads (unless the loop in txaio would be stored in thread local storage - but that seems to be a can of worms). b) covers all use cases (for asyncio). However, it would trigger a lot of change in Autobahn both in impl. and interfaces - with practically zero gain for Autobahn itself, and for most use cases. c) is obviously not so cool. we already have a few places diverged (ApplicationRunner etc), and this doesn't feel right - it's a goal conflict: we want to paper over differences in Twisted/asyncio, without rewrapping everything and expose all framework specific extra features. Mission impossible. In any case, I think this is an important discussion. Right now, I tend to a) .. |
I hadn't really considered a "thread-local" txaio loop (which obviously would just be "the" reactor for Twisted). I wonder: would this cover @soulmerge's use-cases? Twisted I believe realizes the mistake of a global loop (and recent suggested-practice is to take a "reactor" argument for things that require it) -- but yes, it'll be a looong road to multiple reactors "actually" working in Twisted. I think the real mistake here is having an implicit reactor (which, unfortunately, asyncio also has). Pragmatically, right now the only public API in Autobahn that accepts an extra As a refinement of "c" (basically @oberstet implies this mostly) we could do this: allow a I'd still like to see actual open-source asyncio code that does really use multiple loops ;) (Personally, I'd recommend strongly against mixing threads and async code -- especially with WAMP, where you already have good tools to go multi-process which a) avoids confusion and b) gives way better performance). That said, asyncio does give you multiple event-loops out of the box (which is nice) and obviously people are using these even without threads. |
Absolutely. (few exceptions, eg implicit background thread pools to use block database client libraries) Thread local storage: I was more mentioning this for the sake of completeness .. I would strongly argue against introducing that into As far as I understand @soulmerge , it wouldn't be enough anyway: he wants multiple concurrently running loops in one thread - but I might be wrong here.
I like that: essentially, first add support for this in Where else within Minor: should we go into that direction, a trivial, but annoying question: what's the parameter name?
Yes, me too! Because of two reasons:
|
Things that use
(The above for the asyncio side). For Twisted, it's a subset of that -- just the |
The autobahn library's asyncio support is limited by the authors' attempt to support other asyncio libraries: crossbario/autobahn-python#747 These limitations would have prevented us from using score.ws with other asyncio-based Workers. That's why we decided to switch to the websockets library instead.
In addition to the above, the web-socket base protocol uses This would need to changed to be a per-loop batched-timer object. Twisted can use a single batched-timer (as now) and asyncio would need a per-loop implementation (so, push the batched-timer down to |
Just hit this when moving a websocket server from the main thread to a background thread to enable keyboard interrupts to work on Windows. Never heard of txaio prior to this, and was definitely surprised to find its aio.config.loop attribute being used instead of the loop I provided to the factory. In my case there is still only a single loop in use, it just isn't the one from the main thread. Here's the relevant part of the callstack where I discovered what was going wrong: event_loop.py, line 28, in run_loop |
asyncio is deprecating the loop kwarg (for removal in 3.10) throughout its api. It's also not recommended to call
Specifically futures should only be created with: def create_future():
return asyncio.get_running_loop().create_future() |
actually, we only want to determine "the default loop" when the user does not explicitly provides one. the other aspect: yeah, we should create future using the factory present on the loop object for that. and creating a future in autobahn should use the txaio function for that. |
There's a
Future
created by the_consume
method ofautobahn.asyncio.websocket.WebSocketAdapterProtocol
.It is not passing the
loop
kwarg, which leads toNotImplementedError
when the implicit default 'global' loop is not being used. If I understand correctly.I am guessing it's possible to reproduce this simply by creating a custom loop, rather than asking for the default via
asyncio.get_event_loop()
, and passing that to the connection factory.I encountered the issue when creating what amounts to an autobahn websocket connection test using
py.test
(with help from thepytest-asyncio
package that provides some useful asyncio testing fixtures). To repeat that, write a test function to make a websocket connection so that:pytest.mark.asyncio
decorator, with theforbid_global_loop
parameter set toTrue
)event_loop
fixture is used for the test, and explicitly used as the loop passed to autobahnSo the 'stanza' for such a test becomes like this:
Then, upon connect, the NotImplementedError will be raised.
As for fixing the issue, the loop appears to be available in the protocol as
self.transport._loop
, and I can confirm that passing it asloop
kwarg fixes the issue.The text was updated successfully, but these errors were encountered: