mirror of
https://github.com/esphome/esphome.git
synced 2026-09-03 19:46:02 +00:00
Address Copilot review on PR #16021
- download_content_many now de-duplicates by `path` so two callers asking for the same cache file (e.g. the same URL referenced twice in a config) can't race on download_content's non-atomic write. When duplicates are present, the first-seen URL for that path wins. - Clamp `max_workers` to at least 1 so an invalid caller value can't raise ValueError out of ThreadPoolExecutor.
This commit is contained in:
@@ -139,18 +139,31 @@ def download_content_many(
|
||||
Wall time drops from `sum(latency)` to roughly `max(latency)` for cached
|
||||
files where the HEAD round-trip dominates. The first exception raised by
|
||||
any worker is propagated; remaining workers complete before this returns.
|
||||
|
||||
Items are de-duplicated by `path` -- two callers asking for the same
|
||||
cache file (e.g. the same URL referenced twice in a config) would
|
||||
otherwise race on `download_content`'s non-atomic write.
|
||||
"""
|
||||
items = list(items)
|
||||
if not items:
|
||||
seen: dict[Path, str] = {}
|
||||
for url, path in items:
|
||||
if path in seen:
|
||||
continue
|
||||
seen[path] = url
|
||||
if not seen:
|
||||
return
|
||||
if len(items) == 1:
|
||||
url, path = items[0]
|
||||
if len(seen) == 1:
|
||||
path, url = next(iter(seen.items()))
|
||||
download_content(url, path, timeout)
|
||||
return
|
||||
workers = min(max_workers, len(items))
|
||||
workers = max(1, min(max_workers, len(seen)))
|
||||
with ThreadPoolExecutor(max_workers=workers) as ex:
|
||||
# list() forces iteration so exceptions surface here, not silently.
|
||||
list(ex.map(lambda item: download_content(item[0], item[1], timeout), items))
|
||||
list(
|
||||
ex.map(
|
||||
lambda item: download_content(item[1], item[0], timeout),
|
||||
seen.items(),
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
# String constant rather than `from .const import TYPE_WEB` because each
|
||||
|
||||
@@ -350,6 +350,42 @@ def test_download_content_many_propagates_errors(
|
||||
external_files.download_content_many(items)
|
||||
|
||||
|
||||
@patch("esphome.external_files.download_content")
|
||||
def test_download_content_many_dedupes_by_path(
|
||||
mock_download: MagicMock, setup_core: Path
|
||||
) -> None:
|
||||
"""Two items pointing at the same cache path must collapse to one
|
||||
download -- otherwise concurrent writes race on the same file.
|
||||
"""
|
||||
path = setup_core / "shared"
|
||||
items = [
|
||||
("https://example.com/a", path),
|
||||
("https://example.com/b", path),
|
||||
("https://example.com/a", path),
|
||||
]
|
||||
external_files.download_content_many(items)
|
||||
assert mock_download.call_count == 1
|
||||
# First-seen URL wins, matching dict-insertion order.
|
||||
args, _ = mock_download.call_args
|
||||
assert args[0] == "https://example.com/a"
|
||||
assert args[1] == path
|
||||
|
||||
|
||||
@patch("esphome.external_files.download_content")
|
||||
def test_download_content_many_clamps_invalid_max_workers(
|
||||
mock_download: MagicMock, setup_core: Path
|
||||
) -> None:
|
||||
"""`max_workers <= 0` must not raise from ThreadPoolExecutor; it should
|
||||
be clamped up to at least 1 worker.
|
||||
"""
|
||||
items = [
|
||||
("https://example.com/a", setup_core / "a"),
|
||||
("https://example.com/b", setup_core / "b"),
|
||||
]
|
||||
external_files.download_content_many(items, max_workers=0)
|
||||
assert mock_download.call_count == 2
|
||||
|
||||
|
||||
@patch("esphome.external_files.download_content_many")
|
||||
def test_download_web_files_in_config_filters_and_dispatches(
|
||||
mock_many: MagicMock, setup_core: Path
|
||||
|
||||
Reference in New Issue
Block a user