Improve management of PlaylistContainer

[ChrisBrandhorst]

At the moment, a couple of things go wrong concerning the PlaylistContainer:

• If PlaylistContainer ^Session::PlaylistContainer::get() is called when the Session has not logged in yet, an AccesViolationException occurs in the constructor of PlaylistContainer, since the supplied pointer (gotten through sp_session_playlistcontainer) is not valid;
• If the Session is logged out, and then back in, the PlaylistContainer remains empty, and the count always gives -1. This is because the PlaylistContainer apparently is scoped within a Session login timespan (see https://developer.spotify.com/docs/libspotify/12.1.51/group__session.html#ga319767f0b795f1c46a08390b587c5671)

All in all, we need to relate the creation of the PlaylistContainer to the login/logout process of the Session.

A quick fix is nulling the _pc pointer in Session on logout, but this has two disadvantages:

• The PlaylistContainers that were instantiated earlier on are still references somewhere and not GC-ed: they still receive some events from libspotify;
• The client may have references to the 'first' PlaylistContainer. With this fix, this reference silently becomes useless (since it then points to a PlaylistContainer with count -1).

I think the best option is creating the PlaylistContainer just once, but re-linking it to libspotify on Session login/logout, using the appropriate libspotify methods (sp_session_playlistcontainer, sp_playlistcontainer_add_ref, sp_playlistcontainer_add_callbacks, sp_playlistcontainer_remove_callbacks, sp_playlistcontainer_release).

Also, we need to manage the PlaylistContainer when not logged in. I think, with the approach described above, the best option is to be able to retrieve it and throw an Exception if something is done which requires the Session to be logged in.

Your ideas?

[Alxandr]

I disagree (I think) with some of it, and think there's a more deep-rooted problem to be fixed here. But first, let's start with the PLC triggering the loaded callback multiple times. To deal with that (I think) a simple event should be added to the PLC itself, and any calls should be routed to that (including the first), though the first only should set the result of the TCS (this can be achieved by using TrySetResult or manually).

Secondly, I think that we need to add some lifetime management to all the objects that are tied to the Session object (IE. all the objects). A LifeTime manager class should be created (marked internal), and a single instance of this should be given to the Session object. Any and all objects created that depends on the Session (Playlist, Track, PLC, etc.) should (in their constructor, where they take in the session) invoke session->LifeTimeManager->Manage(this, LifeTime.Session); (note that the LifeTime.Session is to indicate that the object should be destroyed when the user signs out. Objects that still makes sense after the user has signed out should be something like LifeTime.LifeTime, and other lifetimes might be added as needed. LifeTime is probably an enum).

The lifetime manager should have a list of weak references to all the objects it managers (IDisposables, we're doing C++/CLI, everything with both the destructors is IDisposable), and whenever we do stuff like signout we signal to the LifeTimeManager that it should dispose of everything that is marked as LifeTime.Session.

Later, we would probably need to add some cleanup to the LifeTimeManager so it doesn't bloat too much, but we can see about that at a later time.

A few things that needs to be dealt with though are the following:

All SpotiFire objects needs to know up front whether or not they are already disposed (they don't need to discern this to the public, and I don't think they should either). Any property access, method call, etc., after a object is disposed should throw an ObjectDisposedException (simple macro). Any call to Dispose after a object has been disposed needs to be ignored. This is important, as a user might dispose a object using a using statement, while logging out inside. A call to IDisposable.Dispose should never throw (unless you get something horrible, such as AccessViolationException, in which case you actually probably want the process to die). Point is, a call to Dispose should never throw an ObjectDisposedException.

The way to make sure objects are only disposed of once are many, though I would prefer if it can be done in a lock free manner. At least, SPLock should not be used. One way is simply to check if the _ptr is anything reasonable (like, not IntPtr.Zero), though I think we create the pointers as initonly (readonly in C#) meaning they cannot be changed to IntPtr.Zero. Though I don't have the code available at the moment, so I might be mistaken on that part.

If, in fact, we do use initonly on the _ptr, I would change that to be not initonly, and use this check to make sure the Dispose functionality is only run once:

IntPtr ptr = Thread::VolatileRead(_ptr);
if(Interlocked::Exchange(_ptr, IntPtr.Zero) == ptr) {
    // dispose goes here.
}

According to my knowledge, this should be perfectly threadsafe, and if anyone thinks me wrong, or has a better solution, please do bring it forth.

@ChrisBrandhorst What do you think?

[ChrisBrandhorst]

Something along these lines has crossed my mind too, so it sounds good to me! Not sure how much I can contribute to this one though, given my skillz... Let's see were I can be of help.