Does `torrent_handle::info_hashes()` race?

[AllSeeingEyeTolledEweSew]

Please provide the following information

libtorrent version (or branch): RC_2_0

platform/architecture: N/A

compiler and compiler version: N/A

https://github.com/arvidn/libtorrent/blob/b1595a0a9898b18e8f8a75b4024ea1dedf2fc8c8/src/torrent_handle.cpp#L215-L219

My understanding of modern C++ is limited, but I don't understand why this function does not have a race condition of copying the info_hash_t, versus it being modified in torrent::set_metadata(). IIUC all the methods in torrent_handle may run in any thread, thus concurrently with anything in the network thread.

Sorry if this is a dumb question.

[arvidn]

I think you're right. There is most likely a data race there. However, I believe it's fixed by https://github.com/arvidn/libtorrent/pull/6923. With that patch the torrent_info object is no longer mutated, but instead replaced.

[AllSeeingEyeTolledEweSew]

I thought this was the offending line (I should have linked it before) https://github.com/arvidn/libtorrent/blob/d99193a52b8d63d6a460e65d012730c2672c6448/src/torrent.cpp#L7581 This line doesn't atomically replace m_info_hash, right? IIUC it invokes operator=() which overwrites the value in a non-atomic way

[arvidn]

the torrent_handle doesn't look at torrent::m_info_hash though. It looks at torrent::m_torrent_file->info_hash(). So the atomic operation happens in the call to torrent::m_torrent_file.lock() which returns a pointer to the (immutable) torrent_info object.

I say immutable, even though some fields of it are mutated by the libtorrent thread, but info_hash() is not one of them (as of that PR).

[AllSeeingEyeTolledEweSew]

the torrent_handle doesn't look at torrent::m_info_hash though. It looks at torrent::m_torrent_file->info_hash()

I disagree, unless I don't understand this sequence:

https://github.com/arvidn/libtorrent/blob/d99193a52b8d63d6a460e65d012730c2672c6448/src/torrent_handle.cpp#L215-L219

https://github.com/arvidn/libtorrent/blob/d99193a52b8d63d6a460e65d012730c2672c6448/include/libtorrent/torrent.hpp#L366

https://github.com/arvidn/libtorrent/blob/d99193a52b8d63d6a460e65d012730c2672c6448/src/torrent.cpp#L7581

[arvidn]

oops, you're right. I was reading t and thinking it was the torrent_info object

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[AllSeeingEyeTolledEweSew]

@arvidn do you think torrent_handle::info_hashes() should be changed to invoke the network thread, and so be synchronized?

[arvidn]

that's quite a pessimization. I'd like to avoid it if possible. It's a gnarly situation though, it's a data race in some cases, but not in the common case.

[AllSeeingEyeTolledEweSew]

It's only safe to use if magnet links are never used. Magnet links are definitely a common case. In the short term, there should at least be a warning in the documentation. We could point to the workaround of using status(), which has the info_hash_t.

It's also a bad code smell that the two unsynchronized methods of torrent_handle have races (also is_valid(), see #5112. I still haven't found any safe way to use this method). Libtorrent should have a coherent concurrency story, and these races make it seem like the story of using torrent_handle accessors is incomplete or ill-considered. In a different issue, it's mentioned that the alert stream isn't a complete story either, because some data can't be accessed via alerts. We ought to complete at least one of these stories.

We shouldn't allow method to have races just because libtorrent would be hard to use otherwise. That just means we need to fix the concurrency story.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.