Skip to content

Commit

Permalink
Fix write_to_file function
Browse files Browse the repository at this point in the history
Probably, because of a racing condition, sometimes the file was
written into inconsistent state which breaks the configuration and
the hardware-exporter service.

After adding a file lock and detaching the permission from writting,
the bug seems to be fixed

Closes: #305
  • Loading branch information
gabrielcocenza committed Oct 11, 2024
1 parent 98b502e commit c92822b
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 16 deletions.
17 changes: 8 additions & 9 deletions src/service.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Exporter service helper."""

import fcntl
import os
from abc import ABC, abstractmethod
from logging import getLogger
Expand Down Expand Up @@ -280,16 +281,14 @@ def write_to_file(path: Path, content: str, mode: Optional[int] = None) -> bool:
"""Write to file with provided content."""
success = True
try:
logger.info("Writing file to %s.", path)
fileobj = (
os.fdopen(os.open(path, os.O_CREAT | os.O_WRONLY, mode), "w", encoding="utf-8")
if mode
# create file with default permissions based on default OS umask
else open(path, "w", encoding="utf-8") # pylint: disable=consider-using-with
)
with fileobj as file:
with open(path, "w", encoding="utf-8") as file:
# Apply a file lock
fcntl.flock(file.fileno(), fcntl.LOCK_EX)
file.write(content)
except (NotADirectoryError, PermissionError) as err:

if mode:
os.chmod(path, mode)
except (NotADirectoryError, PermissionError, OSError) as err:
logger.error(err)
logger.info("Writing file to %s - Failed.", path)
success = False
Expand Down
18 changes: 11 additions & 7 deletions tests/unit/test_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -770,9 +770,10 @@ def setUp(self):
def tearDown(self):
pathlib.Path(self.temp_file.name).unlink()

@mock.patch("service.fcntl")
@mock.patch("builtins.open", new_callable=mock.mock_open)
@mock.patch("service.os")
def test_write_to_file_success(self, mock_os, mock_open):
def test_write_to_file_success(self, mock_os, mock_open, mock_fcntl):
path = pathlib.Path(self.temp_file.name)
content = "Hello, world!"

Expand All @@ -783,22 +784,25 @@ def test_write_to_file_success(self, mock_os, mock_open):

mock_open.assert_called_with(path, "w", encoding="utf-8")
mock_file.write.assert_called_with(content)
mock_os.chmod.assert_not_called()
mock_fcntl.flock.assert_called_once()

@mock.patch("service.os.open", new_callable=mock.mock_open)
@mock.patch("service.os.fdopen", new_callable=mock.mock_open)
@mock.patch("service.fcntl")
@mock.patch("builtins.open", new_callable=mock.mock_open)
@mock.patch("service.os")
def test_write_to_file_with_mode_success(self, mock_os, mock_fdopen, mock_open):
def test_write_to_file_with_mode_success(self, mock_os, mock_open, mock_fcntl):
path = pathlib.Path(self.temp_file.name)
content = "Hello, world!"

mock_file = mock_fdopen.return_value.__enter__.return_value
mock_file = mock_open.return_value.__enter__.return_value

result = service.write_to_file(path, content, mode=0o600)
self.assertTrue(result)

mock_open.assert_called_with(path, mock_os.O_CREAT | mock_os.O_WRONLY, 0o600)
mock_fdopen.assert_called_with(mock_open.return_value, "w", encoding="utf-8")
mock_open.assert_called_with(path, "w", encoding="utf-8")
mock_file.write.assert_called_with(content)
mock_os.chmod.assert_called_with(path, 0o600)
mock_fcntl.flock.assert_called_once()

@mock.patch("builtins.open", new_callable=mock.mock_open)
def test_write_to_file_permission_error(self, mock_open):
Expand Down

0 comments on commit c92822b

Please sign in to comment.