Refactor project structure and architecture

[rastr-0]

• Review and reorganize the code into classes, modules and directories
• Add docstrings (possible can later generate docs from them)

Possible project structure:

bitTorrent_client/
    __init__.py
    core/
        torrent.py
        peer.py
        piece.py
        ...
    network/
        tracker_client.py
        peer_connection.py
    storage/
        file_writer.py
    cli/
        main.py
    utils/
        constants.py
        helpers.py
    test_data/
        test_file1.iso.torrent
        test_file2.iso.torrent

[rastr-0]

Some additional thoughts on the architecture.

• Modules are way too coupled together, if we need to change some core logic one day, we will end up rewriting a lot. For example, adding support for UDP trackers doesn't seem to be implemented without changing almost everything ---> A nice idea could be to introduce typing.Protocol Interfaces in the places, where change is likely to happen.
  For now, I see these places as Storage, Peer and Tracker.
• The project should definitely start moving towards async code, because BitTorrent client is network I/O bound type of application; most of the time it just waits for requests to come ---> async can speed up the project;
• Some classes do what they definitely shouldn't. For example, all the core logic: Piece, Block, PieceManager, Torrent, Message should contain NO I/O logic ---> additional module for handling I/O must be updated (BlockSaver and probably renamed)
• Managers, such as PeerManager, TorrentManager (will be added) should depend on Interfaces, not the actual classes

[rastr-0]

Following the last comment, adding possible workflow:

TorrentManager
 ├── TrackerClient (HTTPTracker)
 │     └── Returns list of peers
 │
 ├── PeerManager
 │     ├── For each peer → TCPPeerConnection
 │     │     └── Handles messages (Piece, Request, Have...)
 │     │
 │     └── Updates PieceManager
 │
 ├── PieceManager
 │     └── Keeps track of piece/block state
 │
 └── Storage (FileStorage)
       └── Writes completed blocks

[rastr-0]

[rastr-0]

Review of Proposed Architecture

The discussion so far is genuinely strong — the proposed layering is correct in direction and the Protocol-based dependency inversion idea is exactly right for this kind of I/O-heavy application. What follows tries to sharpen the edges rather than redirect.

---

On What's Already Proposed

The directory structure in the issue body is solid. The only thing worth reconsidering is placing test_data/ inside the package — test fixtures belong beside your test suite, not inside the importable package.

The Protocol interfaces (Comment 1) are the right call. The current Peer class imports INFO_HASH as a module-level global that gets mutated in main.py (utilities.INFO_HASH = self.torrent.info_hash), which means module import order determines correctness. That's fragile, and Protocols with explicit dependency injection are the right fix.

Async over threading (Comment 1) is correct and worth committing to early. The current design has three Thread subclasses (RunBittorrent, PeerManager, BlockSaver) coordinating through a pubsub bus — a PieceRequestFromPeer event triggers a call from Peer into PeerManager through pub.sendMessage, which then synchronously calls back into PieceManager. That's a callback graph hidden inside a message bus, and it becomes very hard to reason about under load. asyncio makes the concurrency model explicit.

The orchestration hierarchy (Comment 2) is the right mental model. One nuance: TorrentManager should own the lifecycle of all sub-components, including teardown — the current code has no clean shutdown path.

---

Refined Directory Structure

bittorrent/
    __init__.py
    __main__.py              # python -m bittorrent <file.torrent>

    domain/                  # Zero I/O. Pure data + logic.
        block.py             # Block, State enum
        piece.py             # Piece, block state machine
        torrent_meta.py      # TorrentMeta: parses .torrent, computes info_hash — no HTTP
        message.py           # Wire protocol encode/decode

    network/
        tracker.py           # TrackerProtocol + HTTPTracker implementation
        peer_connection.py   # TCPPeerConnection: asyncio reader/writer per peer
        peer_manager.py      # PeerManager: connection pool, peer selection

    storage/
        protocol.py          # StorageProtocol
        file_storage.py      # FileStorage: mmap writes

    orchestration/
        piece_manager.py     # PieceManager: piece/block state, piece selection strategy
        torrent_session.py   # TorrentSession: top-level coordinator (replaces RunBittorrent)

    cli/
        main.py              # Argument parsing, logging setup, session.run()

    exceptions.py            # BitTorrentError hierarchy
    constants.py             # BLOCK_SIZE, HANDSHAKE_PSTR — no mutable state

tests/
    domain/
    network/
    storage/
    conftest.py

test_torrent_files/          # Outside the package, alongside tests

Reasoning for departures from the issue body:

• domain/ replaces core/ to signal the DDD intent more clearly — these classes have zero infrastructure dependencies.
• torrent.py is split into domain/torrent_meta.py (parsing, info_hash computation, filename, piece count) and network/tracker.py (HTTP). Currently torrent.py does both and Torrent.request_to_tracker() calls requests.get — that method does not belong on a domain object.
• orchestration/ separates the coordination layer from both domain and network. PieceManager moves here because it depends on Piece objects but orchestrates state across all of them.
• exceptions.py at the package root makes the error hierarchy visible and importable without circular dependencies.
• constants.py replaces utilities.py. The getter functions (get_pieces_number, get_pieces_hash, etc.) are unnecessary indirection — they each delegate to one attribute and can be removed entirely; callers just use torrent_meta.number_of_pieces directly. The mutable INFO_HASH = None global disappears.
• __main__.py enables python -m bittorrent path/to/file.torrent.

---

Concrete Protocol Definitions

# bittorrent/network/tracker.py
from typing import Protocol
from dataclasses import dataclass

@dataclass(frozen=True)
class PeerAddress:
    ip: str
    port: int

class TrackerProtocol(Protocol):
    async def announce(
        self,
        info_hash: bytes,
        peer_id: bytes,
        downloaded: int,
        uploaded: int,
        left: int,
        event: str,
    ) -> list[PeerAddress]: ...

# bittorrent/storage/protocol.py
from typing import Protocol

class StorageProtocol(Protocol):
    async def write_block(
        self, piece_index: int, block_offset: int, data: bytes
    ) -> None: ...

    async def close(self) -> None: ...

# bittorrent/network/peer_connection.py
from typing import Protocol, AsyncIterator
from bittorrent.domain.message import Message

class PeerProtocol(Protocol):
    """What PeerManager needs from a single peer connection."""
    @property
    def peer_id(self) -> str: ...
    @property
    def pieces(self) -> "BitArray": ...
    @property
    def is_unchoked(self) -> bool: ...

    async def send(self, message: Message) -> None: ...
    async def disconnect(self) -> None: ...

# bittorrent/orchestration/torrent_session.py
class TorrentSession:
    def __init__(
        self,
        meta: TorrentMeta,
        tracker: TrackerProtocol,
        storage: StorageProtocol,
    ) -> None:
        self._meta = meta
        self._tracker = tracker
        self._storage = storage
        self._piece_manager = PieceManager(meta)
        self._peer_manager = PeerManager(self._piece_manager, storage)

    async def run(self) -> None:
        peers = await self._tracker.announce(
            info_hash=self._meta.info_hash,
            peer_id=self._meta.peer_id,
            downloaded=0,
            uploaded=0,
            left=self._meta.file_size,
            event="started",
        )
        await self._peer_manager.connect(peers)
        await self._peer_manager.run()

The key constraint: TorrentSession.__init__ never touches the network. All I/O happens inside run(). This makes the session trivially testable — pass in mock implementations of TrackerProtocol and StorageProtocol.

---

Async Migration Path

A full rewrite in one pass is risky. A staged approach:

Stage 1 — Isolate domain (no async needed). Extract TorrentMeta from torrent.py and move Block, State, Piece, PieceManager into domain/ with zero logic changes except fixing the BLOCK_SIZE import to come from constants.py.

Stage 2 — Replace globals. Delete INFO_HASH = None from utilities.py. Pass info_hash explicitly into PeerConnection.__init__. The peer currently does from utilities import INFO_HASH at import time, so if main.py sets it after the import, peers constructed before that assignment get None. That's a latent bug today.

Stage 3 — Async network layer. Replace Peer (raw socket + select) with asyncio.StreamReader/StreamWriter. The message framing logic in peer.py:get_messages() adapts directly — read 4 bytes for length, then read length bytes for payload:

async def _read_messages(self) -> AsyncIterator[Message]:
    while True:
        header = await self._reader.readexactly(4)
        length, = unpack(">I", header)
        if length == 0:
            continue  # keep-alive
        payload = await self._reader.readexactly(length)
        yield MessageDispatcher(header + payload).dispatch()

Stage 4 — Async storage. Replace the polling BlockSaver thread (while saved_blocks != total) with an asyncio.Queue that PeerConnection pushes completed blocks onto and FileStorage drains. No polling, no BLOCKS_IN_PIECE arithmetic for loop termination.

Stage 5 — Wire in TorrentSession. RunBittorrent(Thread) is gone, replaced by async def main() in cli/main.py calling asyncio.run(session.run()).

---

Latent Bugs Worth Fixing During Refactor

These will surface naturally when you touch these files:

• file_writer.py:24 — while self.saved_blocks != self.number_of_pieces * BLOCKS_IN_PIECE assumes every piece has exactly BLOCKS_IN_PIECE blocks, but the last piece is almost always smaller. The loop can terminate incorrectly for certain file sizes.
• peer_manager.py — except ConnectionRefusedError or ConnectionError evaluates to except ConnectionRefusedError because or on types short-circuits to the first truthy value. Should be except (ConnectionRefusedError, ConnectionError).
• peer.py — INFO_HASH is imported at module load time. If main.py sets utilities.INFO_HASH after importing peer, the peer module already captured None. Explicit injection (Stage 2) fixes this entirely.
• piece_manager.py — SHA1 hash slicing for the last piece may be off-by-one depending on how pieces_hash is indexed in edge cases (single-piece torrents). Worth a unit test during Stage 1.

---

Missing Pieces

• exceptions.py — a typed hierarchy (TrackerError, PeerConnectionError, PieceVerificationError all under BitTorrentError) lets callers distinguish recoverable from fatal errors without catching bare Exception.
• tests/conftest.py — shared fixtures: a parsed TorrentMeta from one of the .torrent files, a mock StorageProtocol. The domain layer being pure (Stage 1) makes unit tests for Piece and PieceManager require zero mocking.
• __main__.py — one liner: from bittorrent.cli.main import main; main().

---

The architecture in the issue is heading in exactly the right direction. The main additions here are the concrete Protocol signatures, the staged migration path that avoids a big-bang rewrite, and the specific bugs that will surface during the refactor anyway.

---

Generated by Claude Code