Add test accessing request.url when transport is closed

[jcugat]

What do these changes do?

I am finding an issue on production when trying to retrieve the request.url property on a closed transport fails. The stacktrace is the following:

    segment.put_http_meta(http.URL, str(request.url))
  File "/usr/local/lib/python3.6/site-packages/aiohttp/helpers.py", line 504, in __get__
    val = self.wrapped(inst)
  File "/usr/local/lib/python3.6/site-packages/aiohttp/web_request.py", line 340, in url
    url = URL.build(scheme=self.scheme, host=self.host)
  File "/usr/local/lib/python3.6/site-packages/aiohttp/helpers.py", line 504, in __get__
    val = self.wrapped(inst)
  File "/usr/local/lib/python3.6/site-packages/aiohttp/web_request.py", line 284, in scheme
    if self.transport.get_extra_info('sslcontext'):
AttributeError: 'NoneType' object has no attribute 'get_extra_info'

I found a similar issue already fixed, but it was for the request.remote property instead: #2474

I'm not really sure what's the best way to fix it, that's why I'm just adding a simple test case to show the problem. Any hint on how to solve it?

This is the test failure:

$ pytest tests/test_web_request.py::test_url_with_closed_transport -vv
Test session starts (platform: darwin, Python 3.6.1, pytest 3.6.4, pytest-sugar 0.9.1)
cachedir: .pytest_cache
rootdir: /Users/josepcugat/workspace/aiohttp, inifile: setup.cfg
plugins: xdist-1.22.5, sugar-0.9.1, mock-1.10.0, forked-0.2, cov-2.5.1


――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― test_url_with_closed_transport ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

>   return inst._cache[self.name]
E   KeyError: 'url'

aiohttp/_helpers.pyx:24: KeyError

During handling of the above exception, another exception occurred:

>   return inst._cache[self.name]
E   KeyError: 'scheme'

aiohttp/_helpers.pyx:24: KeyError

During handling of the above exception, another exception occurred:

    def test_url_with_closed_transport():
        req = make_mocked_request('GET', '/')
        req._protocol = None
>       assert str(req.url).endswith('/')

tests/test_web_request.py:642:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
aiohttp/_helpers.pyx:32: in aiohttp._helpers.reify.__get__
    raise
aiohttp/_helpers.pyx:26: in aiohttp._helpers.reify.__get__
    val = self.wrapped(inst)
aiohttp/web_request.py:351: in url
    url = URL.build(scheme=self.scheme, host=self.host)
aiohttp/_helpers.pyx:32: in aiohttp._helpers.reify.__get__
    raise
aiohttp/_helpers.pyx:26: in aiohttp._helpers.reify.__get__
    val = self.wrapped(inst)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <Request GET / >

    @reify
    def scheme(self) -> str:
        """A string representing the scheme of the request.

            Hostname is resolved in this order:

            - overridden value by .clone(scheme=new_scheme) call.
            - type of connection to peer: HTTPS if socket is SSL, HTTP otherwise.

            'http' or 'https'.
            """
>       if self.transport.get_extra_info('sslcontext'):
E       AttributeError: 'NoneType' object has no attribute 'get_extra_info'

aiohttp/web_request.py:295: AttributeError

 tests/test_web_request.py::test_url_with_closed_transport ⨯                                                                                                                                                                                                                                                                                               100% ██████████

Results (0.18s):
       1 failed
         - tests/test_web_request.py:639 test_url_with_closed_transport

Are there changes in behavior for the user?

No.

Related issue number

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[asvetlov]

Interesting problem :)
We need to know is the connection secured or not.
The check is not trivial. If it is not overridden by plugins like aiohttp-remote lookup for SSL context is needed.
If an underlying socket is closed there is no way to do it.
Getting SSL context on connection establishment is doable but the price is a little performance degradation if the request schema/URL is not needed by request processing.
Maybe the best we can do is raising an explicit error for the case like RuntimeError('connection is closed')

[jcugat]

I've looked at the code again and went for the solution that gets the SSL information at connection establishment. I think this approach is better because for example, in my case, some clients close the connection but I still want to log some things (this is why I try to access the request.url property).

I'm not sure if this will add a lot of latency for every request, but I'm open to other suggestions.

[codecov-io]

Codecov Report

Merging #3177 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3177      +/-   ##
==========================================
- Coverage   98.11%   98.05%   -0.07%     
==========================================
  Files          43       43              
  Lines        7846     7853       +7     
  Branches     1352     1353       +1     
==========================================
+ Hits         7698     7700       +2     
- Misses         57       60       +3     
- Partials       91       93       +2

Impacted Files	Coverage Δ
aiohttp/web_request.py	98.15% <100%> (-0.53%)	⬇️
aiohttp/tcp_helpers.py	90% <0%> (-6.67%)	⬇️
aiohttp/client_reqrep.py	97.47% <0%> (-0.17%)	⬇️
aiohttp/web_middlewares.py	100% <0%> (ø)	⬆️

---

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 4a57522...fe2eb66. Read the comment docs.

[asvetlov]

Well, let's do it

[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