Asyncio RawSocket

[izderadicka]

Hi,

this is my first attempt for rawsocket transport for aio.

Few comments for current code:

• It's python 3 only now - it's bit intentional - since trollius is officially deprecated it does not make much sense for me to support it here - anybody willing to work with asyncio will use python3 anyhow
• I've tried to make it basically independent on remaining code, self-contained - so basically no changes to other parts of code, that can break something else.
• This lead to some duplication - mainly in utils module ( which probably should be shared with remaining codebase) and WampRawSocketMixinGeneral in rawsocket module, which contains exactly same code as rawsocket transport for twisted.
• There is also new runner class ApplicationRunnerRawSocket - again made not to infer with existing code. Potentially it can be easily integrated into current runner - just use additional URI schemes tcp:// and unix:
• no ssl support yet - I do not need it - main motivation was to has backend asyncio component connected on same computer - so unix socket is what I need.
• testing - I've written unittests for this addition. Also tested in python 3 against crossbar - example added. Not sure if you have some more advanced tests - quickly looked to testsuite but it looks like tests are mostly focused on websocket.

Let me know your thoughts - thanks
Ivan

[oberstet]

We use structured logging, not string interpolation

[izderadicka]

You mean like this?

self.log.debug('RawSocker Asyncio: Connection made with peer {peer}', peer=self.peer)

[meejah]

yes, exactly

[oberstet]

Thanks for working on this! As requirement would be Python 3 + 2 support plus all tests green. As you mentioned, you see some potential for avoiding code duplication .. this is highly welcome! Maybe you could break this change up in multiple smaller PRs? Easier/quicker to review ..

[izderadicka]

OK - I'll look into clean CI tests firsts - fixed pep8 already will look into python2

[izderadicka]

I have interesting problem with unittests - in twisted tests I got this error:

===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/ivan/workspace/autobahn-python/.tox/py34-twtrunk/lib/python3.4/site-packages/twisted/trial/runner.py", line 511, in loadPackage
    module = modinfo.load()
  File "/home/ivan/workspace/autobahn-python/.tox/py34-twtrunk/lib/python3.4/site-packages/twisted/python/modules.py", line 389, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "/home/ivan/workspace/autobahn-python/.tox/py34-twtrunk/lib/python3.4/site-packages/twisted/python/reflect.py", line 300, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "/home/ivan/workspace/autobahn-python/.tox/py34-twtrunk/lib/python3.4/site-packages/twisted/python/reflect.py", line 239, in _importAndCheckStack
    return __import__(importName)
  File "/home/ivan/workspace/autobahn-python/autobahn/asyncio/__init__.py", line 36, in <module>
    from autobahn.asyncio.websocket import \
  File "/home/ivan/workspace/autobahn-python/autobahn/asyncio/websocket.py", line 32, in <module>
    txaio.use_asyncio()
  File "/home/ivan/workspace/autobahn-python/.tox/py34-twtrunk/lib/python3.4/site-packages/txaio/__init__.py", line 117, in use_asyncio
    raise RuntimeError("Explicitly using '{}' already".format(_explicit_framework))
builtins.RuntimeError: Explicitly using 'twisted' already

autobahn.asyncio.test.test_rawsocket
-------------------------------------------------------------------------------

It basically happens even before my test module is imported - parent modules autobahn.asyncio is imported first.

Would removing of /autobahn/asyncio/websocket.py", line 32, in
txaio.use_asyncio()

be safe? Because it seems as cause of problem. Or how to exclude searching for tests in autobahn.asycio in this case - I use env.var in test, but this happens before test module is loaded!

[meejah]

@izderadicka regarding the testing, there's a USE_TWISTED=1 environment variable that should control whether the twisted tests are run or not...what might be happening here is that it's effectively trying to run both Twisted and asyncio tests in one process, which won't work.

Removing the .use_asyncio() also won't really work -- that's there so that you're forced to choose twisted vs. asyncio based on imports (e.g. autobahn.twisted.* vs. autobahn.asyncio.*), and it's impossible to do both asyncio and twisted things in the same python process. I will look at the unit-tests in this branch and suggest something useful in the next few hours...

[meejah]

Also, thanks for the PR! This is definitely a nice missing feature to have :)

[izderadicka]

@meejah - I cannot solve problem with unittest just by using USE_TWISTED or USE_ASYNCIO - problem is that when running twisted tests ( for instance tox -c tox.ini -e py34-twtrunk) the test runner still tries to import everything that start with test_ . Which means - if I put my test into autobahn.asyncio.test, where it should logically belong ( at least based on other tests placement in code)' - test runner will try to import it first - which means it'll first import autobahn.asyncio and module's __init__ imports both websocket and wamp submodules, each runs txaio.use_asyncion during the import. So form my perspective I cannot stop this unless I place test in different directory - would this be acceptable - put it for instance in autobahn.test?

[codecov-io]

Current coverage is 59.29%

Merging #679 into master will increase coverage by 1.94%

1. 7 files (not in diff) in autobahn/wamp were modified. more
   • Misses -16
   • Partials -44
   • Hits +60
2. 2 files (not in diff) in autobahn/twisted were modified. more
3. 2 files (not in diff) in autobahn were modified. more
   • Partials -22
   • Hits +22

@@             master       #679   diff @@
==========================================
  Files            63         66     +3   
  Lines         10278      10739   +461   
  Methods           0          0          
  Messages          0          0          
  Branches       1667       1643    -24   
==========================================
+ Hits           5894       6367   +473   
- Misses         3824       3860    +36   
+ Partials        560        512    -48   

Powered by Codecov. Last updated by 1c86fef...f66411e

[izderadicka]

Fixed all issues with clean build. Theoretically should be compatible with python2, but example is for python3 (yield from).
Had to move unit test module - due to issue explained above.
Now decide What to do with Application Runner - I propose to merge with my solution - so it'll be possible to use URLs:

• ws://host:port/path - websocket
• wss://host:port/path - secure websocket
• tcp://host:port - raw socket over TCP
• unix:/path - raw socket over unix domain socket

[meejah]

@izderadicka using the "whole file" os.environ guard like you have already, I can move the file back to autobahn/asyncio/test.

Also, what about something like this: meejah@356bcec

(Seems to work for me, but I'm not entirely sure why the pytest import doesn't fail in e.g. the py27 envs ...). The idea being: bail out early and avoid a "whole file" indent-level...

[izderadicka]

@meejah And it works in py34-twtrunk or in other twisted test environments? It did not for me. The other tip looks fine - nice way how to avoid ident

[meejah]

With changes as on the branch from #679 (comment) all the environments pass for me .. not entirely sure why? Hmm, maybe the "pytest-twisted" req for the Twisted environments ends up installing pytest in them, too...

In any case, I think your first instinct to put these tests in autobahn/asyncio/test/* is the better place -- if the above code works, feel free to add it to this PR (better to do it yourself so it doesn't look like I wrote that whole file ;) by deleting whitespace)...

IMO, the ApplicationRunner stuff is best dealt with in a follow-up PR; then at least we can get this code in and worry about the best syntax for ApplicationRunner separately. FWIW, your proposed URL syntax looks fine -- but there might be implications I'm not thinking of...

[meejah]

p.s. just to clarify explicitly: IMO it's totally fine to have the asyncio example only work with python3 (i.e. "yield from" is fine for the examples).

[izderadicka]

I removed ApplicationRunnerRawSocket.
I really cannot make all tests run if unittest in in autobahn.asyncio.test. Problem is in already in collection phase - because autobahn.asyncio imports from autobahn.asyncio.websocket and autobahn.asyncio.wamp - both packages run txaio.use_asyncio() on top level. So it's run during collection - it does not matter that all test is skipped.
Try this (trial is used to run tests for twisted):

USE_TWISTED=1 trial  autobahn

or this

USE_TWISTED=1 py.test --twisted -k 'not asyncio/test' autobahn

Both fail for me because of imports in collection phase. Before it was not problem because there were no tests in autobahn.asyncio.

It about organization of imports - question: are imports in autobahn.asyncio package needed? Why for instance import there form websocket, if I'll use only rawsocket. But that's for you to decide. From my perspective I've tried implementation that will not disrupt existing code.

@meejah - if you really can make test working in autobahn.asyncio.test I'll need more detailed guidance - I've tried both proposed ways, with no luck. But as I explained I think problem is elsewere.

[meejah]

@izderadicka Ah, okay, I was missing a __init__ in asyncio/test which is why these appeared to be working for me. With the __init__.py then I can repeat your failures as per the above comment.

I'll see if I can make it work, but for now let's stick with the one which does work -- i.e. putting the tests in the "non ideal" place; we can always fix it later if there's a nicer way. Alternatively, if you'd prefer to keep them in the "ideal" place, you can keep it under the if os.environ(..) instead.

[izderadicka]

@meejah I see - that's cool trick - if test is not package, then it'll not import parent packages. I leave decision to you as maintainers - what is smaller evil - test in non-ideal place or in right place, but not as package ( all other test directories are packages - e.g. have __init__.py)

[meejah]

I'm still not really sure what's going on, specifically why the Twisted tests work fine with a similar setup (including __init__.py). Paging @hawkowl as IIRC she did "something" to make this work nicely ...

[meejah]

Ah, I think I know what's happening: the mere presence of a test_*.py file anywhere below twisted.asyncio.* causes trial to import twisted.asyncio which itself causes txaio.use_asyncio() to be called (after we already selected twisted). But, I think I have a solution that allows asycio tests in "the right spot".

[meejah]

See izderadicka#1

[izderadicka]

Merged @meejah solution for unittest. Two more things were needed - adding autobahn.asyncio.test as package_data to setup.py and also modifying coverage command in tox.ini - because otherwise coverage data are not collected.

Frankly spoken after playing a while with tests, maybe the simplest solution was not to try to put this test into autobahn.asyncio. Longer term solution could be to have all tests separate from package as proposed here http://pytest.org/latest/goodpractices.html#goodpractices
(now we combine inline tests with and without init file, which causes some issues).

[meejah]

Personally, I like separating tests from "real code". Some people prefer the opposite.

But it might make the whole asyncio vs twisted vs generic test-organization a lot nicer to do something like:

• autobahn/* (existing code, as now)
  • twisted/*.py
  • asyncio/*.py
• tests/* (move test-code to here)
  • /generic/test_*.py
  • /asyncio/test_*.py
  • /twisted/test_*.py

I'm not sure if this would remove all the hack-ish things to get around test-collection etc. That is, trial would still have to be "tricked" into not importing tests/asyncio/* unless we make tests/* "not a module" (i.e. no __init__). Then we could be explicit when running things and eliminate the environment-variables (i.e. run like trial tests/generic tests/twisted or py.test tests/generic/ tests/asyncio).

There's also the "distribute the test-code with your module or not" question (e.g. that's one of the reasons people prefer tests "in" with the code, so they always come along). Since Autobahn has "small" systems as a target audience this might be a good reason to not include that code at all (i.e. you only get the tests/* code if you do a Git checkout; a pip install wouldn't include them).

Maybe for this PR @izderadicka's original solution is really the best? :/

...and we could consider a wholesale move of the tests to their own tree as a separate ticket?

[oberstet]

@izderadicka cool! thanks a lot!