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

Server closes connection if wrong type returned by handler #4572

Closed
Fogapod opened this issue Feb 13, 2020 · 7 comments · Fixed by #8845
Closed

Server closes connection if wrong type returned by handler #4572

Fogapod opened this issue Feb 13, 2020 · 7 comments · Fixed by #8845
Labels
bug need pull request reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR server

Comments

@Fogapod
Copy link
Contributor

Fogapod commented Feb 13, 2020

🐞 Describe the bug

Server closes session if handler returns anything else that Response instance.

💡 To Reproduce

from aiohttp import web


async def handler(req):
    return 1

app = web.Application()
app.add_routes([web.get("/", handler)])
web.run_app(app)
~ curl localhost:8080
curl: (52) Empty reply from server

💡 Expected behavior

Error message in console and returned status 500.

📋 Your version of the Python

$ python --version
Python 3.8.1

📋 Your version of the aiohttp/yarl/multidict distributions

Tested with aiohttp 3.6.2 and master branch fresh installations.

@Fogapod Fogapod added the bug label Feb 13, 2020
@webknjaz webknjaz added server need pull request reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR labels Feb 13, 2020
@webknjaz
Copy link
Member

@Fogapod thanks for the report! It'd be cool if you could submit a failing test marked with xfail in a PR as per https://pganssle-talks.github.io/xfail-lightning.

@Fogapod
Copy link
Contributor Author

Fogapod commented Feb 13, 2020

@webknjaz alright, I'll try to make a test for this issue.

I found that returning HTTPException instance in aiohttp 3.6.2 resulted in returning corresponding status code by server, in master branch it results in closed connection.
Is returning exceptions deprecated? I didn't find it in documentation, there is no traceback in console.

@webknjaz
Copy link
Member

I think it is, or should be. Exceptions must be raised.

@Fogapod
Copy link
Contributor Author

Fogapod commented Feb 13, 2020

RuntimeError is raised here

raise RuntimeError("Web-handler should return "
"a response instance, "
"got {!r}".format(resp))

after calling
resp, reset = await task

and catched here
except RuntimeError as exc:
if self._loop.get_debug():
self.log_exception(
'Unhandled runtime exception', exc_info=exc)

For some reason these are only logged in asyncio debug mode.

I'm not sure how this could be fixed. Returning status 500 for all Runtime errors?

@webknjaz
Copy link
Member

Returning status 500 for all Runtime errors?

I think so

webknjaz pushed a commit that referenced this issue Feb 14, 2020
PR #4574 by @Fogapod

This change documents a contract of disallowing non-response
return values in request handlers that currently has a bug and
is therefore marked as xfail.

Ref: #4572
@webknjaz
Copy link
Member

Is returning exceptions deprecated? I didn't find it in documentation, there is no traceback in console.

v3.6 deprecates returning exceptions but it still works (see the red warning box): https://docs.aiohttp.org/en/stable/web_quickstart.html#exceptions

master/v4.0 doesn't support this anymore and doesn't mention returning exceptions: https://docs.aiohttp.org/en/latest/web_exceptions.html

@Fogapod
Copy link
Contributor Author

Fogapod commented Feb 14, 2020

I don't understand this condition:

if self._loop.get_debug():
self.log_exception(
'Unhandled runtime exception', exc_info=exc)

What kind of exceptions can happen there and why they are silenced by default?
If there are exceptions that should be silenced and cause server to close connection without response, what could a workaround be? Raising different kind of exception from handler if wrong type returned?

Edit: according to this comment #1790 (comment), runtime exceptions should not be handled, but in this case it can be, so exception type should be changed?

asvetlov pushed a commit that referenced this issue Oct 16, 2020
PR #4574 by @Fogapod

This change documents a contract of disallowing non-response
return values in request handlers that currently has a bug and
is therefore marked as xfail.

Ref: #4572
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug need pull request reproducer: present This PR or issue contains code, which reproduce the problem described or clearly understandable STR server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants