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.

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

Extract TrackCollectionManager from Library #2365

Merged
merged 45 commits into from
Dec 8, 2019
Merged

Extract TrackCollectionManager from Library #2365

merged 45 commits into from
Dec 8, 2019

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Nov 21, 2019

As discussed on Zulip

  • Extract TrackCollectionManager from Library.
  • Route all modifying requests of Tracks through TrackCollectionManager to keep both the internal TrackCollection and all external track collections synchronized.
  • Fix shutdown order and process to avoid spurious crashes in GlobalTrackCache when exiting the application, caused by delayed destruction of internal TrackPointer instances in various components.
  • Only formal, no functional changes besides bug fixing!

Marks as [WiP] to allow rebasing on master. Otherwise merge conflicts may arise and complicate the review of the actual changes.

@uklotzde uklotzde changed the title [WiP] Extrack TrackCollectionManager from Library [WiP] Extract TrackCollectionManager from Library Nov 21, 2019
@daschuer
Copy link
Member

Marks as [WiP] to allow rebasing on master. Otherwise merge conflicts may arise and complicate the review of the actual changes

So this should not be reviewed yet?
In case of conflicts you can just merge master and resolve conflicts. GitHub shows than only the original changes and this can still be reviewed.

@uklotzde uklotzde changed the title [WiP] Extract TrackCollectionManager from Library Extract TrackCollectionManager from Library Nov 21, 2019
build/depends.py Outdated Show resolved Hide resolved
build/depends.py Outdated Show resolved Hide resolved
build/depends.py Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

Thanks for trying to clean up the database access.

This compiles and all tests still pass. However, I get a segfault when starting Mixxx:

[...]
ALSA lib pcm_dmix.c:1108:(snd_pcm_dmix_open) unable to open slave
ALSA lib pcm_dmix.c:1108:(snd_pcm_dmix_open) unable to open slave
connect(2) call to /dev/shm/jack-1000/default/jack_0 failed (err=No such file or directory)
attempt to connect to server failed
Warning [Main]: ControlDoublePrivate::getControl returning NULL for ( "[AutoDJ]" , "enabled" )
Segmentation Fault (core dumped)

$ coredumpctl -1 info
           PID: 153595 (mixxx)
           UID: 1000 (jan)
           GID: 1000 (jan)
        Signal: 11 (SEGV)
     Timestamp: Thu 2019-11-21 13:57:32 CET (52s ago)
  Command Line: ./mixxx
    Executable: /data/jan/Projects/mixxx/cmake_build/mixxx
 Control Group: /user.slice/user-1000.slice/session-1.scope
          Unit: session-1.scope
         Slice: user-1000.slice
       Session: 1
     Owner UID: 1000 (jan)
       Boot ID: 7db033707c384c9ca8d05dc25469917d
      Hostname: jan-desktop
       Storage: /var/lib/systemd/coredump/core.mixxx.1000.7db033707c384c9ca8d05dc25469917d.153595.1574341052000000000000.lz4 (truncated)
       Message: Process 153595 (mixxx) of user 1000 dumped core.

                Stack trace of thread 153595:
                #0  0x00007ff566ef8534 n/a (n/a)

src/library/library.h Outdated Show resolved Hide resolved
src/library/trackloader.h Outdated Show resolved Hide resolved
src/mixxx.cpp Show resolved Hide resolved
src/mixxx.cpp Show resolved Hide resolved
Co-Authored-By: Be <be.0@gmx.com>
@uklotzde
Copy link
Contributor Author

uklotzde commented Nov 25, 2019

69faf56 fixes a double free issue that was caused by introducing parented_ptr in MixxxMainWindows. Maybe this could be the cause for the deadlock?

operator T*()in parented_ptr is convenient but dangerous, because it allows to invoke delete on wrapped pointers without any warning!

@daschuer
Copy link
Member

Yes, maybe there is s way to delete the delete operator.

void operator delete (parented_ptr<T> ptr) = delete

Or delete the void* cast

operator void*() = delete

@uklotzde
Copy link
Contributor Author

Overloading operator delete does not work.

@daschuer
Copy link
Member

Why not in our case? I have just read that it should work.

Did you try as member function

void operator delete(void* ptr) = delete;

Or keep it and do nothing in the body.

@uklotzde
Copy link
Contributor Author

Ok, then this might be a special case for QMainWindow. The debug assertion in the destructor of parented_ptr was triggered.

But, anyway, deleting a parented_ptr is unintended. Pointers should be managed and owned only by a single authority.

@uklotzde
Copy link
Contributor Author

Now you have to explicitly obtain the plain pointer from parented_ptr if your want to delete it manually.

@uklotzde
Copy link
Contributor Author

To use the function pointer variant of QMetaObject::invokeMethod() was not really helpful. I didn't check myself that this has been added "recently" in Qt 5.10 and is not available in earlier versions.

@uklotzde
Copy link
Contributor Author

Build errors are fixed.

I'm not able to reproduce any deadlock on shutdown.

@daschuer
Copy link
Member

I will watch this a bit and if this does not appear again, we can merge.

@uklotzde uklotzde added this to the 2.3.0 milestone Nov 29, 2019
@Be-ing
Copy link
Contributor

Be-ing commented Dec 7, 2019

Ping @daschuer, ready for merge? Let's not keep this open indefinitely.

@daschuer
Copy link
Member

daschuer commented Dec 8, 2019

I did some more test without any deadlock. So we can merge it now.
Thank you @uklotzde

@daschuer daschuer merged commit d31c18b into mixxxdj:master Dec 8, 2019
@uklotzde
Copy link
Contributor Author

uklotzde commented Dec 8, 2019

Thank you all for your efforts and trust. I'll try to keep the following PRs smaller for easier review.

@uklotzde uklotzde deleted the dev_library branch December 8, 2019 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants