response return value runtime check

[benitogf]

#3311

[codecov-io]

Codecov Report

Merging #3321 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3321      +/-   ##
==========================================
+ Coverage      98%   98.01%   +0.01%     
==========================================
  Files          43       43              
  Lines        8018     8025       +7     
  Branches     1355     1356       +1     
==========================================
+ Hits         7858     7866       +8     
  Misses         66       66              
+ Partials       94       93       -1

Impacted Files	Coverage Δ
aiohttp/helpers.py	97.46% <100%> (ø)	⬆️
aiohttp/web_protocol.py	92.52% <100%> (+0.49%)	⬆️
aiohttp/client_reqrep.py	97.49% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 564c43f...3f6020b. Read the comment docs.

[asvetlov]

The check should be isinstance(resp, StreamResponse).
Please move it outside of try/except just before resp.prepare() call.
In check reraise a RuntimeError exception to log it one level upper and close the connection to client.
The check should be performed in debug mode only for sake of performance.

[asvetlov]

Also, a test is required to never miss the functionality after code refactoring.

[benitogf]

The check should be performed in debug mode only for sake of performance.

I don't follow this part, how is checking if we are in debug mode different from checking the resp type?

w/o debug check:

                    if isinstance(resp, StreamResponse):
                        await resp.prepare(request)
                        await resp.write_eof()
                    else:
                        raise RuntimeError('Possibly missing return ' +
                                        ' statement on request handler')

with debug check:

                if self._loop.get_debug():
                    if isinstance(resp, StreamResponse):
                        await resp.prepare(request)
                        await resp.write_eof()
                    else:
                        raise RuntimeError('Possibly missing return ' +
                                        ' statement on request handler')
                else:
                    await resp.prepare(request)
                    await resp.write_eof()

[asvetlov]

                if self.debug:
                    if inot sinstance(resp, StreamResponse):
                        raise RuntimeError('Web-handler should return a response instance, got {!r}'.format(resp))
                await resp.prepare(request)
                await resp.write_eof()

[asvetlov]

Almost done.

I have no idea what did you wrong with git but your PR has not only your changes but many unrelated fixes as well.
I suspect it is a result of rebasing.

Note: aiohttp uses squash-and-merge strategy for applying PRs.
It means you don't need to rebase, merge from master only if needed.
Guthub manages the rebasing pretty well.

[asvetlov]

No need for the check, self.force_close() implies setting self.transport to None

[benitogf]

this check prevents the log.debug last value being overwritten by: https://github.com/aio-libs/aiohttp/pull/3321/files/8d585c826f9fb6fc82a5291f5973caf136aaabee#diff-8ac506a3012476288a758f156ca7d7b2R449 and fail the assert: https://github.com/aio-libs/aiohttp/pull/3321/files/8d585c826f9fb6fc82a5291f5973caf136aaabee#diff-98519d3deafe019f57304a557d20531cR542

maybe there's a way to test it w/o relying on the last log.debug value?

[asvetlov]

Got you.
Let's drop self.log_debug("Possibly missing return statement on request handler") log but log RuntimeError with it's message.

'Web-handler should return a response instance, got {!r}' is a good enough hint to the missing return statement, isn't it?

[benitogf]

nope, that runtimeError will be bypassed by this one: https://github.com/aio-libs/aiohttp/pull/3321/files/8d585c826f9fb6fc82a5291f5973caf136aaabee#diff-8ac506a3012476288a758f156ca7d7b2R439 another reason why I used log.debug

[asvetlov]

Ok. Agree

[asvetlov]

Please replace \ with implicit strings concatenation as a line above.

[asvetlov]

thanks!

[lock]

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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new