Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mtime reset when assigning unchanged values to Item fields #2664

Closed
wisp3rwind opened this issue Aug 21, 2017 · 6 comments
Closed

mtime reset when assigning unchanged values to Item fields #2664

wisp3rwind opened this issue Aug 21, 2017 · 6 comments
Labels
bug bugs that are confirmed and actionable

Comments

@wisp3rwind
Copy link
Member

Problem

In library.py

class Item(LibModel):
    # ...
    def __setitem__(self, key, value):
        """Set the item's value for a standard field or a flexattr.
        """
        # Encode unicode paths and read buffers.
        if key == 'path':
            if isinstance(value, six.text_type):
                value = bytestring_path(value)
            elif isinstance(value, BLOB_TYPE):
                value = bytes(value)

        if key in MediaFile.fields():
            self.mtime = 0  # Reset mtime on dirty.

        super(Item, self).__setitem__(key, value)

the mtime will be reset (and consequently Item._dirty be set) irrespective of whether any field actually
changed. Most of the time, this is probably not a problem since the item would be dirtied anyway, and even if not, no real database corruption will occur. There is a particular situation where many unnecessary mtime updates and metadata writes to the mediafiles can be triggered, though:

Into the following beet edit session, the query selected a full album, but I only changed one song title:

beet -v edit txarango album:'som riu'                                                                                                                                                                      
...
Sending event: library_opened
edit: invoking editor command: ['/usr/bin/nvim', '/tmp/tmp9l8hedej.yaml']
Txarango - Som riu - Esperança
  title: Esperança -> Esperança [Not a remix]
continue [E]diting, Apply, Cancel? a
edit: saving changes to Txarango - Som riu - Esperança [Not a remix]
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/01 - Esperança.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Músic de carrer
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/02 - Músic de carrer.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - La vuelta al mundo
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/03 - La vuelta al mundo.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Compta amb mi
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/04 - Compta amb mi.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - El meu poble
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/05 - El meu poble.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Alegre i encantada
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/06 - Alegre i encantada.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Som un riu
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/07 - Som un riu.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Governant
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/08 - Governant.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Batega
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/09 - Batega.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Corazón viajero
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/10 - Corazón viajero.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Camp de batalla
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/11 - Camp de batalla.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Com dues gotes d'aigua
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/12 - Com dues gotes d'aigua.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - Tant de bo
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/13 - Tant de bo.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
edit: saving changes to Txarango - Som riu - No t'adormis
Sending event: write
Sending event: after_write
moving /.../Txarango/Som riu/14 - No t'adormis.mp3 to synchronize path
the: "Txarango" -> "Txarango"
Sending event: before_item_moved
Sending event: item_moved
Sending event: database_change
Sending event: database_change
Sending event: database_change
Sending event: cli_exit

but still all of the items are committed to the database and have their metadata written out to the files. This happens because the edit plugin assigns all fields whether they were changed or not, and then checks Item._dirty and if it is non-empty calls Item.try_sync. Given that the mtime is always changed, the _dirty check is useless.

While the edit plugin should probably not re-assign unchanged fields in the first place, it seems to me that the behaviour of Item.__setitem__ is not ideal either. I'm not really sure about the implications for beets overall, but a first (untested) "fix" could be the following (Item._dirty is just a set()):

class Item(LibModel):
    # ...
    def __setitem__(self, key, value):
        """Set the item's value for a standard field or a flexattr.
        """
        # Encode unicode paths and read buffers.
        if key == 'path':
            if isinstance(value, six.text_type):
                value = bytestring_path(value)
            elif isinstance(value, BLOB_TYPE):
                value = bytes(value)

        old_dirty = self._dirty.copy()
        
        super(Item, self).__setitem__(key, value)

        if old_dirty != self._dirty and key in MediaFile.fields():
            self.mtime = 0  # Reset mtime on dirty.

Maybe copying this set() all the time has performance implications and there's a better way?

Any thoughts on how __setitem__ should behave? Or would it be safer to only change the edit plugin?
@sampsyo
Copy link
Member

sampsyo commented Aug 22, 2017

Hmm! This is a very good point. Thanks for the thorough investigation—it made the problem easy to understand.

I think you’re right that we should change the behavior of Item.__setitem__ here. It seems to be more logically consistent for the “cleanliness” behavior and the mtime handling to agree with each other. More interactions beyond just the edit plugin could benefit from this—for example, no-op modify actions will get the same benefit.

Here’s one possible fix: in dbcore, we can make a new method that works sort of like __setitem__. (Perhaps it will be called set or assign.) But unlike __setitem__, it will also return a Boolean indicating whether a change was actually made (i.e., whether dirty changed). Then we can call that method from Model.__setitem__ and from here.

That seems like it could be a pretty simple (but effective) change, if I’m envisioning the necessary code correctly.

@wisp3rwind
Copy link
Member Author

Factoring this into a separate Model method is certainly a lot cleaner than duplicating Model._dirty. I have implemented this locally (the tests pass) as Model._setitem (with the leading underscore since I couldn't come up with a reason why any higher level access would need to modify the mtime updates):
Feel free to suggest a better name, though.

I'll open a PR once I've come up with a test for the edit CLI.

@sampsyo
Copy link
Member

sampsyo commented Aug 22, 2017

That sounds great! Woohoo!

The tests in test_edit.py are quite intricate—it's a tricky thing to orchestrate. Thanks for wading through that.

@wisp3rwind
Copy link
Member Author

Turns out that there's a test already... but it's broken due to the way test fixtures are set up :/

@wisp3rwind wisp3rwind mentioned this issue Aug 22, 2017
Merged
@arcresu
Copy link
Member

arcresu commented Apr 20, 2019

It looks like the linked PR fixed this issue. Can we close this @wisp3rwind?

@arcresu arcresu added the bug bugs that are confirmed and actionable label Apr 20, 2019
@wisp3rwind
Copy link
Member Author

Yes, I just read through the PR and its commits again and it looks like all of this was fixed. Thanks for the reminder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug bugs that are confirmed and actionable
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sampsyo @arcresu @wisp3rwind