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

🐛 Dy-Sidecar fix use wait_for instead of async_timeout for running subtasks #3129

Merged

Conversation

sanderegg
Copy link
Member

What do these changes do?

This PR fixes an issue where an asynchronous task is not properly cancelled when a timeout occurs.

Be very careful when using such constructs as:

try:
  async with async_timeout.timeout(delay=1):
    result = await some_long_running_task()
except asyncio.TimeoutError:
  # IMPORTANT NOTE: This will not cancel the task here. It must be manually done!

Instead in such case use

try:
  await asyncio.wait_for(some_long_running_task(), timeout=1)
except asyncio.TimeoutError:
  # HERE we are safe, the task was properly cancelled for us!

One issue that was found, is that httpx.clients were issuing PoolTimeout errors. These happen when all the connections in the pool are used, which should NEVER happen.

Related issue/s

How to test

import asyncio
import httpx

GG_TEST_URL = "https://www.google.com"
MS_TEST_URL = "https://www.microsoft.com"

timeout = httpx.Timeout(3.0, connect=5.0, pool=1.0)
limits = httpx.Limits(max_connections=5)

import async_timeout


async def get_url_with_async_timeout(client, url):
    # This is not cancelling tasks
    try:
        async with async_timeout.timeout(delay=0.1):
            return await client.get(url)
    except asyncio.TimeoutError:
        print(f"calling {url} timed-out")
        return "timed-out"


async def get_url_with_asyncio(client, url):
    # this is cancelling tasks
    try:
        return await asyncio.wait_for(client.get(url), timeout=0.1)
    except asyncio.TimeoutError:
        print(f"calling {url} timed-out")
        return "timed-out"


async def test():
    async with httpx.AsyncClient(timeout=timeout, limits=limits) as client:
        tasks = {
            asyncio.create_task(get_url_with_asyncio(client, GG_TEST_URL))
            for _ in range(10)
        } | {
            asyncio.create_task(get_url_with_asyncio(client, MS_TEST_URL))
            for _ in range(10)
        }

        # Wait for first one to terminate.
        done, pending = await asyncio.wait(tasks, return_when=asyncio.FIRST_COMPLETED)
        print(client._transport._pool.connections)

        await asyncio.wait(tasks, return_when=asyncio.ALL_COMPLETED)
        print(client._transport._pool.connections)

        for task in tasks:
            assert task.done()
            response = task.result()
            if isinstance(response, httpx.Response):
                print(f"response from {response.url}", response.status_code)
            else:
                print(f"reponse is {response}")

        wiki_response = await client.get("https://www.wikipedia.com") # works nicely with asyncio, badly with async-timeout
        print(f"response from {wiki_response.url}", wiki_response.status_code)


if __name__ == "__main__":
    asyncio.run(test())

Checklist

@sanderegg sanderegg requested review from GitHK and pcrespov as code owners June 22, 2022 09:19
@sanderegg sanderegg self-assigned this Jun 22, 2022
@sanderegg sanderegg added this to the Diolkos milestone Jun 22, 2022
Copy link
Member

@mrnicegyu11 mrnicegyu11 left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #3129 (b72d2a0) into master (827e2a0) will decrease coverage by 0.0%.
The diff coverage is 82.3%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3129     +/-   ##
========================================
- Coverage    81.0%   81.0%   -0.1%     
========================================
  Files         706     706             
  Lines       30490   30489      -1     
  Branches     3949    3949             
========================================
- Hits        24705   24703      -2     
  Misses       4941    4941             
- Partials      844     845      +1     
Flag Coverage Δ
integrationtests 65.9% <80.6%> (+<0.1%) ⬆️
unittests 77.2% <76.4%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...service_director_v2/api/routes/dynamic_services.py 87.2% <68.7%> (-1.1%) ⬇️
..._director_v2/modules/dynamic_sidecar/client_api.py 76.0% <80.0%> (ø)
...2/src/simcore_service_director_v2/core/settings.py 96.1% <100.0%> (ø)
...or_v2/models/schemas/dynamic_services/scheduler.py 98.4% <100.0%> (ø)
...ector_v2/modules/dynamic_sidecar/scheduler/task.py 80.3% <100.0%> (-1.1%) ⬇️
.../src/simcore_service_dynamic_sidecar/core/utils.py 79.0% <100.0%> (-0.6%) ⬇️
.../web/server/src/simcore_service_webserver/utils.py 51.5% <0.0%> (-3.2%) ⬇️
...ce_webserver/studies_dispatcher/_studies_access.py 88.1% <0.0%> (-3.0%) ⬇️
.../director/src/simcore_service_director/producer.py 62.3% <0.0%> (+0.2%) ⬆️
... and 2 more

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Excellent!

@pcrespov pcrespov requested a review from GitHK June 22, 2022 15:05
@sanderegg sanderegg force-pushed the dy-sidecar/bugfix/bad_timeout branch from afda743 to ad4133c Compare June 22, 2022 15:11
@sanderegg sanderegg requested a review from pcrespov June 22, 2022 15:11
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sanderegg sanderegg merged commit d1d8ed3 into ITISFoundation:master Jun 23, 2022
@sanderegg sanderegg deleted the dy-sidecar/bugfix/bad_timeout branch June 23, 2022 15:51
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.

5 participants