vimpunk / cratetorrent

A BitTorrent V1 engine library for Rust (and currently Linux)
473 stars 35 forks source link

Acknowledge that TorrentIds are globally unique #94

Closed ExceptionallyHandsome closed 3 years ago

ExceptionallyHandsome commented 3 years ago

This code probably doesn't do what it's supposed to:

https://github.com/mandreyel/cratetorrent/blob/dbb3719c776f0b74ac6ea1aabcbb724d08078127/cratetorrent/src/lib.rs#L168-L185

What caught my attention is that lazy_static doesn't really do anything here. static TORRENT_ID: AtomicU32 = AtomicU32::new(0); would do the same thing.

But the real concern is that the comments suggest different behaviour.

I don't recommend issuing per-whatever IDs. What if, for example, process A creates let a = ID(0), process B creates let b = ID(0), then b is send to the process A? Just issuing global u32 ids is free from this problem.

vimpunk commented 3 years ago

Hey!

First of all, thanks for sending this PR!

Good catch on the lazy static: I think I had initially used a different type that didn't have a const constructor.

As for the torrent id uniqueness: you're correct that multiple instances of a cratetorrent binary (which is what I meant by process) may have clashing ids.

However, the reason it was not addressed in the MVP is that there is currently no functionality in which a torrent's id is able to meaningfully escape the process, nor is it meant to. E.g. a torrent's state is not persisted to disk, nor are the ids sent via IPC anywhere (within the engine anyway, I suppose the library user might do something like that, but in that case the responsibility of ensuring correctness is delegated to the user).

Once torrents are able to be resumed (by persisting their state to disk), this will be more relevant, and the id generation will have to be revamped.

Anyway, the corrections in the comments are welcome, you're right that they are misleading. Thanks!