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 send_timeout, close_timeout, and send_x timeout parameter #2537

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

frederikaalund
Copy link
Contributor

What do these changes do?

See the title and the discussion in #2310.

Are there changes in behavior for the user?

No. The old timeout parameter will take precedence over the new close_timeout parameter. The default close_timeout of 10.0 seconds is still in place.

Related issue number

#2310

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.bug)
    • 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."

@frederikaalund
Copy link
Contributor Author

frederikaalund commented Nov 19, 2017

I tried writing a functional test for the send_x calls. However, I failed to do so. Seemingly, the send_x calls return immediately despite my best efforts to stall them. I know from experience that the send_x can stall in practice (#2309) but I'm unable to simulate these conditions in a test setting.

Here is my attempt (that doesn't work):

async def test_send_bytes_timeout(loop, test_client):
    closed = loop.create_future()
    raised = False

    async def handler(request):
        ws = web.WebSocketResponse(send_timeout=0.1)
        await ws.prepare(request)

        try:
            # The send_bytes call returns immediately.
            # I also tried with 1 GB of data to defeat buffering. That didn't work.
            await ws.send_bytes(b'0')
            # Calling drain doesn't help either.
            await ws.drain()
        except asyncio.TimeoutError:
            nonlocal raised
            raised = True

        closed.set_result(1)
        await ws.close()
        return ws

    app = web.Application()
    app.router.add_get('/', handler)
    client = await test_client(app)
    async with client.ws_connect('/') as ws:
        await closed
    assert raised

@codecov-io
Copy link

codecov-io commented Nov 19, 2017

Codecov Report

Merging #2537 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2537      +/-   ##
==========================================
+ Coverage   97.56%   97.56%   +<.01%     
==========================================
  Files          40       40              
  Lines        8085     8088       +3     
  Branches     1413     1413              
==========================================
+ Hits         7888     7891       +3     
  Misses         74       74              
  Partials      123      123
Impacted Files Coverage Δ
examples/web_ws.py 95.47% <0%> (+0.04%) ⬆️

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 81594ec...9147863. Read the comment docs.

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Would you write tests for expired timeouts?

Should we use a timeout for ping and pong?
Pretty sure yes.

Also please add send_timeout etc to client_ws.py -- there is no reason to have no such functionality in client web socket as well.

self._receive_timeout = receive_timeout
self._send_timeout = send_timeout
self._close_timeout = timeout or close_timeout
Copy link
Member

Choose a reason for hiding this comment

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

Please deprecate timeout.
Something like

if timeout is not None:
    warnings.warn("timeout parameter is deprecated, use close_timeout instead", DeprecationWarning)
    close_timeout = timeout

@@ -846,7 +846,8 @@ Response
WebSocketResponse
^^^^^^^^^^^^^^^^^

.. class:: WebSocketResponse(*, timeout=10.0, receive_timeout=None, \
.. class:: WebSocketResponse(*, timeout=None, receive_timeout=None, \
send_timeout=None, close_timeout=10.0, \
Copy link
Member

Choose a reason for hiding this comment

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

Please describe added params below.

@asvetlov
Copy link
Member

asvetlov commented Dec 4, 2017

@frederikaalund ping

@frederikaalund
Copy link
Contributor Author

Sorry about the delay. I'll try to find time to implement the changes this week. Thanks for the detailed review.

@asvetlov
Copy link
Member

asvetlov commented Dec 4, 2017

No problem!

@aio-libs aio-libs deleted a comment from CLAassistant Nov 21, 2020
@Dreamsorcerer Dreamsorcerer added the pr-unfinished The PR is unfinished and may need a volunteer to complete it label Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-unfinished The PR is unfinished and may need a volunteer to complete it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants