Skip to content
This repository was archived by the owner on Jan 21, 2022. It is now read-only.
This repository was archived by the owner on Jan 21, 2022. It is now read-only.

Is calling “DoesTorrentExist” twice on a torrent necessary? #274

Open
Open
Is calling “DoesTorrentExist” twice on a torrent necessary?#274
@issuefiler

Description

@issuefiler
issuefiler
opened on May 10, 2021
FATAL   Could not add new torrent to the database
ERROR: duplicate key value violates unique constraint

magnetico/cmd/magneticod/main.go

Lines 110 to 118 in 9c2183b

exists, err := database.DoesTorrentExist(infoHash[:])
if err != nil {
zap.L().Fatal("Could not check whether torrent exists!", zap.Error(err))
} else if !exists {
metadataSink.Sink(result)
}
case md := <-metadataSink.Drain():
if err := database.AddNewTorrent(md.InfoHash, md.Name, md.Files); err != nil {

What’s the point of having the DoesTorrentExist call there when it allows multiple torrents to be Sinked before the AddNewTorrent call? With that, I still need to call DoesTorrentExist again in AddNewTorrent, otherwise it’ll eventually crash on identical torrents that have been double-Sinked.

You’re aware of this

magnetico/pkg/persistence/sqlite3.go

Lines 122 to 157 in 9c2183b

// Although we check whether the torrent exists in the database before asking MetadataSink to
// fetch its metadata, the torrent can also exists in the Sink before that:
//
// If the torrent is complete (i.e. its metadata) and if its waiting in the channel to be
// received, a race condition arises when we query the database and seeing that it doesn't
// exists there, add it to the sink.
//
// Do NOT try to be clever and attempt to use INSERT OR IGNORE INTO or INSERT OR REPLACE INTO
// without understanding their consequences fully:
//
// https://www.sqlite.org/lang_conflict.html
//
// INSERT OR IGNORE INTO
// INSERT OR IGNORE INTO will ignore:
// 1. CHECK constraint violations
// 2. UNIQUE or PRIMARY KEY constraint violations
// 3. NOT NULL constraint violations
//
// You would NOT want to ignore #1 and #2 as they are likely to indicate programmer errors.
// Instead of silently ignoring them, let the program err and investigate the causes.
//
// INSERT OR REPLACE INTO
// INSERT OR REPLACE INTO will replace on:
// 1. UNIQUE or PRIMARY KEY constraint violations (by "deleting pre-existing rows that are
// causing the constraint violation prior to inserting or updating the current row")
//
// INSERT OR REPLACE INTO will abort on:
// 2. CHECK constraint violations
// 3. NOT NULL constraint violations (if "the column has no default value")
//
// INSERT OR REPLACE INTO is definitely much closer to what you may want, but deleting
// pre-existing rows means that you might cause users loose data (such as seeder and leecher
// information, readme, and so on) at the expense of /your/ own laziness...
if exist, err := db.DoesTorrentExist(infoHash); exist || err != nil {
return err
}

  • It should either prevent the double-Sinking of the same torrent with the DoesTorrentExist call at the AddNewTorrent removed, or let AddNewTorrent solely do the DoesTorrentExist check. DoesTorrentExist calls are expensive, taking about 50 milliseconds.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions