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

Add Request.stream #697

Merged
merged 7 commits into from May 24, 2017
Merged

Add Request.stream #697

merged 7 commits into from May 24, 2017

Conversation

38elements
Copy link
Contributor

@38elements 38elements commented May 6, 2017

This adds Request.stream and stream decorator, as below.

from sanic import Sanic
from sanic.views import CompositionView
from sanic.views import HTTPMethodView
from sanic.views import stream as stream_decorator
from sanic.blueprints import Blueprint
from sanic.response import stream, text

bp = Blueprint('blueprint_request_stream')
app = Sanic('request_stream')


class SimpleView(HTTPMethodView):

    @stream_decorator
    async def post(self, request):
        result = ''
        while True:
            body = await request.stream.get()
            if body is None:
                break
            result += body.decode('utf-8')
        return text(result)


@app.post('/stream', stream=True)
async def handler(request):
    async def streaming(response):
        while True:
            body = await request.stream.get()
            if body is None:
                break
            body = body.decode('utf-8').replace('1', 'A')
            response.write(body)
    return stream(streaming)


@bp.put('/bp_stream', stream=True)
async def bp_handler(request):
    result = ''
    while True:
        body = await request.stream.get()
        if body is None:
            break
        result += body.decode('utf-8').replace('1', 'A')
    return text(result)


async def post_handler(request):
    result = ''
    while True:
        body = await request.stream.get()
        if body is None:
            break
        result += body.decode('utf-8')
    return text(result)

app.blueprint(bp)
app.add_route(SimpleView.as_view(), '/method_view')
view = CompositionView()
view.add(['POST'], post_handler, stream=True)
app.add_route(view, '/composition_view')


if __name__ == '__main__':
    app.run(host='127.0.0.1', port=8000)

@r0fls
Copy link
Contributor

r0fls commented May 6, 2017

does this replace #696?

@38elements 38elements mentioned this pull request May 6, 2017
@38elements
Copy link
Contributor Author

38elements commented May 6, 2017

I am sorry.
Yes, this does.

for i in range(1, 250000):
data += str(i)
request, response = app.test_client.post('/stream', data=data)
text = data.replace('1', 'A')
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

request handler replaces '1' with 'A'.
This makes the correct result.

Copy link
Contributor Author

@38elements 38elements May 6, 2017

Choose a reason for hiding this comment

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

Thank you for reviewing.
I removed replacing '1' with 'A'.

sanic/app.py Outdated
@@ -27,7 +27,7 @@
class Sanic:

def __init__(self, name=None, router=None, error_handler=None,
load_env=True, request_class=None,
load_env=True, request_class=None, is_request_stream=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes more sense to set self.is_request_stream = True in the app.stream method, if used. This would avoid having to initialize it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

So around line 170

Copy link
Contributor Author

@38elements 38elements May 7, 2017

Choose a reason for hiding this comment

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

I think your pointing out is correct.
I think it is better to add self.is_request_stream = True in app.route() for blueprint.stream().

if stream:
    self.is_request_stream = True

@38elements
Copy link
Contributor Author

38elements commented May 7, 2017

There is a bug.
This can not use CompositionView and HTTPMethodView.

AttributeError: 'function' object has no attribute 'is_stream'

I will fix it and add tests.

@38elements
Copy link
Contributor Author

I add stream in composition_view.add() and test for CompositionView and HTTPMethodView.

@38elements
Copy link
Contributor Author

38elements commented May 7, 2017

I am sorry for adding code.
I added stream decorator for HTTPMethodView.

@38elements
Copy link
Contributor Author

There is a bug.
CompositionView and HTTPMethodView can not set self.is_request_stream = True.
It is necessary to set True to self.is_request_stream in app.add_route() for CompositionView and HTTPMethodView .
I will fix it and add tests.

@38elements
Copy link
Contributor Author

I fixed it and added tests.

@seemethere
Copy link
Member

seemethere commented May 8, 2017

So instead of it's own decorator wouldn't it make sense to make it as just a parameter that you can pass in through the regular method decorators?

Example:

@app.post('/help/<id>', stream=True)
def stream_handler(request):
    pass

Not necessarily a show-stopper just asking the question.

Great work as always @38elements

@38elements
Copy link
Contributor Author

Thank you for reviewing.
Since I think it is more consistent, I think it is better stream parameter than stream decorator.
I will delete stream decorator and add stream parameter and change tests.

@38elements
Copy link
Contributor Author

I replaced stream decorator to stream parameter.
Only post, put and patch decorator have stream argument.

@seemethere
Copy link
Member

Will merge once I create a milestone for this

@38elements
Copy link
Contributor Author

Thank you.

@38elements
Copy link
Contributor Author

I added a heading in streaming.md.

@r0fls
Copy link
Contributor

r0fls commented May 19, 2017

we should also remove this from the todo here: https://github.com/channelcat/sanic/blob/master/README.rst#todo

@38elements
Copy link
Contributor Author

@r0fls
I think the line should be removed by #733.

@seemethere seemethere merged commit 48de321 into sanic-org:master May 24, 2017
@seemethere seemethere added this to the 0.6.0 milestone May 24, 2017
@gsvijayraajaa
Copy link

gsvijayraajaa commented Jun 18, 2017

Trying to execute the code shared by @38elements .
Getting the error:

from sanic.views import stream as stream_decorator
ImportError: cannot import name 'stream'

Is the code merged to master ?

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

Successfully merging this pull request may close these issues.

None yet

4 participants