From bacbe5d4bb58dfd405e3f331976137e64b96c6c8 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Thu, 14 Apr 2022 15:05:19 +0200 Subject: [PATCH 1/3] drivers: prevent client crash if GET /drivers is closed from client side When a HTTP client sends a query but closes the socket before an answer is received, aiohttp signals it on the server end by raising an asyncio.CancelledError in the associated query handler. By default, when a task is cancelled with asyncio, the task(s) that it is currently awaiting on are cancelled as well. The GET handler for /drivers?wait=true awaits on the "list drivers" task. Therefore, if the GET handler gets cancelled, so will be the "list drivers" task. When that happens, any subsequent call to GET /drivers?wait=true will make the server raise an asyncio.CancelledError because the "list drivers" task has already been cancelled. This results in: * the socket being closed from the server end * an aiohttp.client_exceptions.ServerDisconnectedError exception raised on the client end. This type of exception is unhandled and makes the client crash. Fixed by preventing the "list drivers" task from being cancelled when the GET /drivers query handler gets cancelled. https://bugs.launchpad.net/subiquity/+bug/1968729 Signed-off-by: Olivier Gayot --- subiquity/server/controllers/drivers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subiquity/server/controllers/drivers.py b/subiquity/server/controllers/drivers.py index ed6d9468..96e6285d 100644 --- a/subiquity/server/controllers/drivers.py +++ b/subiquity/server/controllers/drivers.py @@ -91,7 +91,7 @@ class DriversController(SubiquityController): async def GET(self, wait: bool = False) -> DriversResponse: if wait: - await self._drivers_task + await asyncio.shield(self._drivers_task) return DriversResponse(install=self.model.do_install, drivers=self.drivers) From 7310658de62bc40a160722bf2d9034b4f203622b Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 14 Apr 2022 12:05:46 -0600 Subject: [PATCH 2/3] test/api: add drivers cancel test --- subiquity/tests/api/test_api.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index c002d1da..779c4f2a 100755 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -8,6 +8,7 @@ import json import os import tempfile import unittest +from unittest.mock import patch from urllib.parse import unquote from subiquitycore.utils import astart_command @@ -912,3 +913,26 @@ class TestRegression(TestAPI): data.pop('gap') await inst.post('/storage/v2/edit_partition', data) # should not throw an exception complaining about boot + + +class TestCancel(TestAPI): + @timeout() + async def test_cancel_drivers(self): + with patch.dict(os.environ, {'SUBIQUITY_DEBUG': 'has-drivers'}): + async with start_server('examples/simple.json') as inst: + names = ['locale', 'keyboard', 'source', 'network', 'proxy', + 'mirror', 'storage'] + await inst.post('/meta/mark_configured', endpoint_names=names) + await inst.get('/meta/status', cur='WAITING') + await inst.post('/meta/confirm', tty='/dev/tty1') + await inst.get('/meta/status', cur='NEEDS_CONFIRMATION') + + try: + await asyncio.wait_for(inst.get('/drivers', wait=True), + 0.001) + except asyncio.exceptions.TimeoutError: + pass + + # should not raise ServerDisconnectedError + resp = await inst.get('/drivers', wait=True) + self.assertEqual(['nvidia-driver-470-server'], resp['drivers']) From 962ea117c45eb327f7e2a02b02780ec9515722dc Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Thu, 21 Apr 2022 11:53:06 +0200 Subject: [PATCH 3/3] test/api: rely on drivers endpoint to block until APT is configured Signed-off-by: Olivier Gayot --- subiquity/tests/api/test_api.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index 779c4f2a..b3dfd667 100755 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -920,6 +920,12 @@ class TestCancel(TestAPI): async def test_cancel_drivers(self): with patch.dict(os.environ, {'SUBIQUITY_DEBUG': 'has-drivers'}): async with start_server('examples/simple.json') as inst: + # /drivers?wait=true is expected to block until APT is + # configured. + # Let's make sure we cancel it. + with self.assertRaises(asyncio.TimeoutError): + await asyncio.wait_for(inst.get('/drivers', wait=True), + 0.1) names = ['locale', 'keyboard', 'source', 'network', 'proxy', 'mirror', 'storage'] await inst.post('/meta/mark_configured', endpoint_names=names) @@ -927,12 +933,6 @@ class TestCancel(TestAPI): await inst.post('/meta/confirm', tty='/dev/tty1') await inst.get('/meta/status', cur='NEEDS_CONFIRMATION') - try: - await asyncio.wait_for(inst.get('/drivers', wait=True), - 0.001) - except asyncio.exceptions.TimeoutError: - pass - # should not raise ServerDisconnectedError resp = await inst.get('/drivers', wait=True) self.assertEqual(['nvidia-driver-470-server'], resp['drivers'])