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

Added Python 3.5 “async for” compat to websocket #543

Merged
merged 1 commit into from
Oct 24, 2015

Conversation

flying-sheep
Copy link
Contributor

Instead of

while True:
    msg = await ws.receive()
    ...
    elif msg.tp == web.MsgType.close:
        break

do:

async for msg in ws:
    ...

@flying-sheep
Copy link
Contributor Author

uh, those test failures are no my fault. i fixed them in #544

@flying-sheep
Copy link
Contributor Author

triggering rebuild

@flying-sheep flying-sheep reopened this Oct 2, 2015
@flying-sheep
Copy link
Contributor Author

hmm, OK, now the error is:

$ flake8 aiohttp
aiohttp/web_ws.py:297:19: F821 undefined name 'StopAsyncIteration'

but this is wrong. of course this is an undefined name in python < 3.5, but as you’d have to call ws.__anext__() manually in those versions, and you don’t do that because it’s a magic method wrapping receive.

so i’ll just make flake8 shut up.

@asvetlov
Copy link
Member

asvetlov commented Oct 3, 2015

Tests and documentation update are required

@flying-sheep
Copy link
Contributor Author

i don’t seem to get this test to run. it hangs, don’t know why…

@carlcarl
Copy link
Contributor

carlcarl commented Oct 4, 2015

In Travis log: aiohttp/web_ws.py:295:1: W293 blank line contains whitespace.
There are spaces at line 295.
(The testing use flake8 to check the coding style at the beginning)

@flying-sheep
Copy link
Contributor Author

i know, i just made a last-minute fix and didn’t run it after 😐

check the new one, that’s the real problem.

@flying-sheep flying-sheep force-pushed the patch-3 branch 2 times, most recently from 401f1e3 to 48ae89e Compare October 4, 2015 12:49
@carlcarl
Copy link
Contributor

carlcarl commented Oct 4, 2015

Seems hangs on test_web_websocket.py line 42:

msg = await resp._reader.read()

In resp._reader.read(), check streams.py line 374:

yield from self._waiter

It stops there I guess...

@flying-sheep
Copy link
Contributor Author

ya. why?

@carlcarl
Copy link
Contributor

carlcarl commented Oct 5, 2015

Here is the part of the my testing in test_await:

    items = ['q1', 'q2', 'q3']
    for item in items:
        resp._writer.send(item)
        msg = await resp._reader.read()
        assert msg.tp == websocket.MSG_TEXT
        assert item + '/answer' == msg.data

    resp._writer.close()

    msg = await resp._reader.read()
    assert msg.tp == websocket.MSG_CLOSE
    assert msg.data == 1000
    assert msg.extra == ''

    await closed
    await resp.close()

Server will not know if it need to send close message back. So you need to send close first or it will wait for receive forever.

Also, remember to await resp.close()

@flying-sheep
Copy link
Contributor Author

thanks! you saved me quite some time. 🙇

i copied the test from elsewhere and adapted it … poorly.

i hope this is it then!

Instead of

while True:
	msg = await ws.receive()
	...
	elif msg.tp == web.MsgType.close:
		break

do:

async for msg in ws:
	...

items = ['q1', 'q2', 'q3']
for item in items:
resp._writer.send(item)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need private attribute access. The test may use send() and receive() client response methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, no idea, i copied stuff from elsewhere. what methods are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resp._writer.send(item) ?
I think he means _writer is a private attribute and should not be used in testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. my question is what i should use instead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asvetlov asvetlov merged commit c3da779 into aio-libs:master Oct 24, 2015
@flying-sheep flying-sheep deleted the patch-3 branch October 25, 2015 13:12
@flying-sheep
Copy link
Contributor Author

oh, sorry. i would have done it eventually 😅

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants