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

Process manager should not attempt to unlink the processes database file in case of OperationalError #7932

Conversation

kozlovsky
Copy link
Contributor

This PR fixes #7931 by handling all OperationalError exceptions before DatabaseError exceptions, and by suppressing possible PermissionError exceptions when trying to remove the database filw after the DatabaseError

@kozlovsky kozlovsky force-pushed the fix/process_manager_connect_permission_error branch 2 times, most recently from f34d807 to 06f49d5 Compare March 18, 2024 12:44
@kozlovsky kozlovsky marked this pull request as ready for review March 18, 2024 12:53
@kozlovsky kozlovsky force-pushed the fix/process_manager_connect_permission_error branch from 06f49d5 to 89bee0d Compare March 18, 2024 16:03
@kozlovsky kozlovsky requested review from a team and drew2a and removed request for a team March 19, 2024 10:57
Comment on lines 304 to 314
class TestError(Exception):
pass


@patch('sqlite3.connect', MagicMock(side_effect=sqlite3.DatabaseError('error text')))
@patch('pathlib.Path.unlink', MagicMock(side_effect=TestError))
def test_connect_database_error_raise_non_permission_error(process_manager):
# All OperationalError exceptions except "unable to open database file" should be re-raised
with pytest.raises(TestError):
with process_manager.connect():
pass # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: The TestError class could be defined inside the test_connect_database_error_raise_non_permission_error function since it's only used there. This will increase the readability of the entire test file.

Suggested change
class TestError(Exception):
pass
@patch('sqlite3.connect', MagicMock(side_effect=sqlite3.DatabaseError('error text')))
@patch('pathlib.Path.unlink', MagicMock(side_effect=TestError))
def test_connect_database_error_raise_non_permission_error(process_manager):
# All OperationalError exceptions except "unable to open database file" should be re-raised
with pytest.raises(TestError):
with process_manager.connect():
pass # pragma: no cover
@patch('sqlite3.connect', MagicMock(side_effect=sqlite3.DatabaseError('error text')))
@patch('pathlib.Path.unlink', MagicMock(side_effect=TestError))
def test_connect_database_error_raise_non_permission_error(process_manager):
class TestError(Exception):
pass
# All OperationalError exceptions except "unable to open database file" should be re-raised
with pytest.raises(TestError):
with process_manager.connect():
pass # pragma: no cover

@kozlovsky kozlovsky force-pushed the fix/process_manager_connect_permission_error branch from 89bee0d to 5d6bf31 Compare March 19, 2024 11:55
@kozlovsky kozlovsky force-pushed the fix/process_manager_connect_permission_error branch from 5d6bf31 to c87dfbc Compare March 19, 2024 12:53
@kozlovsky kozlovsky merged commit 288f3d7 into Tribler:release/7.13 Mar 19, 2024
17 checks passed
@kozlovsky kozlovsky deleted the fix/process_manager_connect_permission_error branch March 19, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants