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

separate frontend and backend #111

Merged
merged 14 commits into from Jan 15, 2018

Conversation

Projects
None yet
2 participants
@Trolldemorted
Copy link
Contributor

Trolldemorted commented Nov 3, 2017

In the last few days I have been playing around to find a decent way to update an arbitrary number of MainPageViewModels from our background tasks.

This is not ready to be merged (as it does not support sending or opening additional MainPages yet), but the basic design should™ easily allow it:
SignalLibHandle is a singleton which acts as a gatekeeper between the frontend(s)/bg task and the library, and is enforcing stable ordering of all operations. New windows may register their dispatcher and their MPVM (AddWindow(CoreDispatcher d, ISignalWindow w)), and will be notified of any occuring event:

public interface ISignalWindow
{
    void AddOrUpdateConversation(SignalConversation conversation);
    void HandleIncomingMessage(SignalMessage message);
    void HandleIdentitykeyChange(LinkedList<SignalMessage> messages);
}

If the bg task is using the SignalLibHandle, it can notice that MainWindowDispatcher is null, and could call a bg-task-interface's methods.

What do you think so far?

@Trolldemorted Trolldemorted force-pushed the multiwindow branch 7 times, most recently from ab45e11 to 339fe89 Nov 4, 2017

@Trolldemorted

This comment has been minimized.

Copy link
Contributor Author

Trolldemorted commented Nov 6, 2017

A few nights without sleep later we can now open an arbitrary number of windows, whose states should™ be perfectly synchronized! 🎉 🎉 🎉 You can spawn a new window on a desktop by right-clicking on the taskbar icon and then left-clicking "Signal Private Messenger".

things to test

  • multiple windows, obviously
  • Is the performance of the SignalLibHandle system sufficient on mobile devices?

things to consider

  • On which platforms do we want to enable the multi window feature?
  • Should we provide more obvious/easy ways to spawn a new window? E.g. dragndrop a conversation?
  • This most likely breaks on pages other than the MainPage since i did not find time to look at them so far. In which states/on what pages do we want to allow additional windows to be opened?
  • What do we have to change so that the lib is easily usable by a bg process?
  • I think I broke some mobile behaviour (which was previously fixed in e99e55c). What exactly was the problem with and happens during suspension?
  • I broke group info requests. Will repair as soon as I find the time!
  • I may (read: definitely) have broken behaviour on non-MainPage pages. Will conduct triage after work tomorrow.

cc @golf1052 @SERVCUBED

this.InitializeComponent();
this.Suspending += OnSuspending;
this.Resuming += OnResuming;

This comment has been minimized.

@golf1052

golf1052 Nov 6, 2017

Contributor

Need to add this back for the MessagePipe to start up again on mobile.

deferral.Complete();
}

private async void OnResuming(object sender, object e)

This comment has been minimized.

@golf1052

golf1052 Nov 6, 2017

Contributor

As well as here.

@golf1052

This comment has been minimized.

Copy link
Contributor

golf1052 commented Nov 6, 2017

Just quickly looked over this. This is very similar to the refactor I did to support a background task. #108 is actually just 1 of 2 parts. Both parts were done, I just wanted to break it up into a manageable PR. Probably should have mentioned I also refactored the MessagePipe to not depend on the MainPageVM.

Definitely take a look at the background-thread branch because they do very similar things. The even model will work just as well, the only thing we would need to be careful of would be locking around things that should be locked like DB saves but this is already done.

@Trolldemorted

This comment has been minimized.

Copy link
Contributor Author

Trolldemorted commented Nov 6, 2017

Should we make the handle re-usable? Since it currently tracks our windows we might want to, as the view models call it frequently. This way we should be able to shutdown/startup our libsignal tasks w/o changing the frontend project at all.

Yeah I took a look at #108, that's why the branches are so similar :) How will the event model work, though? I didn't see any handling of dispatchers, and without the whole thing is gonna blow as soon as we want to update a secondary window.

@golf1052

This comment has been minimized.

Copy link
Contributor

golf1052 commented Nov 8, 2017

I'm not sure how the event based model fails with updating multiple views (which is where dispatchers come into play) but I guess this is due to my unfamiliarity with multiple views.

But I also may be unclear about what this change intends, it seems you want to support multiple independent windows instead of just the ability to spawn new views from one running app.

The one problem I see with supporting multiple independent windows is that we only want 1 instance of the app talking to Signal and updating the DB.

What I'm guessing happens with multiple views (based on looking over this) is that the view can be spawned and will I assume call OnNavigatedTo like the main view. I think multiple views makes sense for conversations, being able to pop out multiple conversation views while leaving the conversation thread picker in the main window makes sense. This also should still work with the event model, would need some more refactoring in the background-thread branch though.

When a conversation is selected the conversation view would subscribe to the relevant events. That conversation view would update it's view from fired events. When the conversation is deselected or the conversation view is closed then the events are unsubscribed.

I'm not currently seeing why the part that receives messages from Signal has to know about the view. When a message is received save it to the DB and then fire an event saying a new message is available. This way when there is no view everything still works the same. For the background task, instead of updating a view, we send out a notification instead.

@Trolldemorted

This comment has been minimized.

Copy link
Contributor Author

Trolldemorted commented Nov 8, 2017

Isn't supporting multiple windows and spawning new views in a new frame exactly the same?

Multiple windows are not meant only for single conversations. I use multiple virtual desktops, and it is very annoying that clicking on a notification throws me back to the one I opened Signal-Windows on.

Letting only one window talk with signal and update the db is not enough: We must ensure that all windows have the same state and display exactly the same content, so we must also synchronize displaying and creating messages too (if you send messages while receiving at the same time, we have to enforce the same order of messages in all windows). Thus both display->save->send and receive->save->display must be guarded by the same lock, and every display update in every window must be finished within the lock's protection, otherwise the ordering is racy (and very very bad things will happen. Our conversation view depends on knowing exactly how many messages are supposed to be there, allowing racy updates is not an option.

Dynamic registering of events makes things more complicated: If we don't want to be notified by literally everything, we have to keep track on what a window registered if we want to shut it down in a clean way.

Also, we sometimes need immediate feedback from all windows: When a message comes in, we must know whether the corresponding thread is open, scrolled at the bottom and its window has focus before we can device if we want to dispatch a notification or not (TODO: use this to insert the message with the correct read state to reduce db usage).

There are indeed ways that make events work with multiple windows, but I am not convinced going that way is easier. The blog post is a great read on multiple windows, though! I used that post to get started with the multiple windows mindset.

Long story short, if we can guarantee that

  • we can hold the handle lock while dispatching the display/newmessage events without running into deadlocks
  • the viewmodels are not touched while an event handler is running (i.e. no await in the event handlers, or more locking)
  • the event handlers are executed with the correct CoreDispatcher/SynchronizationContext
  • we leak no memory by having stall registered event handlers
  • the event handlers run parallel
  • we can get feedback from them
  • blocking for an event's completion with the handle lock held does not cause deadlocks

we can go the event way, but I fear that will require a lot more code and very precise proofreading and testing.

@Trolldemorted Trolldemorted force-pushed the multiwindow branch from 339fe89 to 7284956 Nov 19, 2017

@Trolldemorted Trolldemorted force-pushed the multiwindow branch from adddc7b to dcfe9c7 Dec 7, 2017

@Trolldemorted Trolldemorted referenced this pull request Jan 5, 2018

Closed

support multiple windows #117

10 of 10 tasks complete

@Trolldemorted Trolldemorted force-pushed the multiwindow branch from c058343 to faae3de Jan 5, 2018

@golf1052

This comment has been minimized.

Copy link
Contributor

golf1052 commented Jan 12, 2018

I've given up on the event based version of this because events don't let us do these

  • the event handlers run parallel
    • Possible but harder
  • we can get feedback from them
    • Can't do

What is left on this PR before we can merge it in? I've gotten the background thread working in my branch so I can bring that into here.

@Trolldemorted

This comment has been minimized.

Copy link
Contributor Author

Trolldemorted commented Jan 12, 2018

my list of known issues:

  1. the create new contact page doesn't work atm because it wants to talk to the signal server directly
  2. messages may arrive before the first AddWindow call, so we won't display notifications for these.
  3. frame.goback is creating new views and viewmodels (and iirc we cannot forbid that). Still not sure how we are gonna handle this - create an "interior force-cached" viewmodel and have the "real" viewmodels re-use them?
  4. signallibhandle needs support for dispatching conversation changes (see the UpdateConversation skeleton), and the add contact and conversation settings pages needs to call it
  5. if a new group arrives if multiple windows are open, a marshal exception arises. Looks like we bind to the same object, and I suspect the same will happen with new contacts:
[UI] [Warning] [Signal_Windows.Lib.IncomingMessages] HandleIncomingMessages() failed in line 0: One or more errors occurred. (Eine Schnittstelle, die für einen anderen Thread marshalled war, wurde von der Anwendung aufgerufen. (Exception from HRESULT: 0x8001010E (RPC_E_WRONG_THREAD)))
   at System.Threading.Tasks.Task.WaitAll(Task[] tasks, Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.WaitAll(Task[] tasks, Int32 millisecondsTimeout)
   at System.Threading.Tasks.Task.WaitAll(Task[] tasks)
   at Signal_Windows.Lib.SignalLibHandle.DispatchAddOrUpdateConversation(SignalConversation conversation)
   at Signal_Windows.Lib.SignalLibHandle.SaveAndDispatchSignalMessage(SignalMessage message, SignalConversation conversation)
   at Signal_Windows.Lib.IncomingMessages.HandleSignalMessage(SignalServiceEnvelope envelope, SignalServiceContent content, SignalServiceDataMessage dataMessage, Boolean isSync, Int64 timestamp)
   at Signal_Windows.Lib.IncomingMessages.HandleMessage(SignalServiceEnvelope envelope)
   at Signal_Windows.Lib.IncomingMessages.OnMessage(SignalServiceMessagePipeMessage message)
   at libsignalservice.SignalServiceMessagePipe.ReadBlocking(IMessagePipeCallback callback)
   at Signal_Windows.Lib.IncomingMessages.HandleIncomingMessages()
  1. this PR alone does not make Signal-Windows fit to allow db changes while it is loaded in memory. To be more specific, the MPVMs do not yet update their conversations in memory when resuming because so far they never had to. I propose we add something like void HandleNewConversationList(List<SignalConversation> to the ISignalWindow interface. This would also make our life easier if we want to register the first window in the SignalLibHandle's constructor call, which would make mitigating issue 2 We have to ensure we clear all caches and decide what to do with open conversations
  2. we might want to rename some stuff. Most functions/classes got their names in a prototyping phase, so they are not the best. E.g. ISignalWindow should be something like ISignalFrontend or ISignalConsumer? Some renaming has been done, open for proposals!
  3. we might want to come up with something that allows Signal-Windows to interrupt the bg task. Imagine you didn't have internet for a longer period, so the background process is pulling down lots of messages, and the UI won't start because it cannot grab the SignalLibHandle because the bg task is holding it
  4. New windows are always spawned on the virtual desktop where the main (?) window is, and not on the virtual desktop the window was requested
  5. We have to disable multiwindow for mobile
@Trolldemorted

This comment has been minimized.

Copy link
Contributor Author

Trolldemorted commented Jan 14, 2018

  1. was fixed by 9881f3f!

Shall we merge and sort out the remaining issues piece by piece? That would avoid this merge from growing even bigger, but the master will not be production-ready.

@golf1052

This comment has been minimized.

Copy link
Contributor

golf1052 commented Jan 15, 2018

Yeah I feel like it would be better to just merge it in now. As long as it's in a mainly stable state.

refactor ISignalWindow to ISignalFrontend, reimplement parts of the s…
…tartup code, fix some marshallingexceptions caused by multiple bindings, centralize multi view storage in App
@Trolldemorted

This comment has been minimized.

Copy link
Contributor Author

Trolldemorted commented Jan 15, 2018

It should be stable unless you click the add contact button :shipit:

Fixed some of the issues, will create one/some issues tomorrow, its 3am right now :(

@Trolldemorted Trolldemorted merged commit 8075ed3 into master Jan 15, 2018

@golf1052

This comment has been minimized.

Copy link
Contributor

golf1052 commented Jan 15, 2018

For the background thread to process incoming messages we're going to need to use events. Windows Runtime classes can only implement Windows Runtime interfaces. Meaning we could move the interface into the background thread project but that would be a weird hierarchy. Events however work fine.

@Trolldemorted

This comment has been minimized.

Copy link
Contributor Author

Trolldemorted commented Jan 15, 2018

What? Why? What about standard interfaces like IList? Can you push your current state so I can have a look?

@golf1052

This comment has been minimized.

Copy link
Contributor

golf1052 commented Jan 15, 2018

Check out the stuff in MultiwindowTesting. If you try to make RandomNumberPuller (a Runtime component) try to inherit INumber you'll get an error upon build. You can inherit from IRuntimeInterface fine however.

@Trolldemorted

This comment has been minimized.

Copy link
Contributor Author

Trolldemorted commented Jan 15, 2018

Ah, WinRT libs must only export WinRT Types. Whatever floats their boat, I think we can just use a not exported implementation of the interface like this:

internal class BGNumber : INumber
{
    public void Numbering()
    {
        throw new NotImplementedException();
    }
}

and have that class call our components' methods.

@golf1052 golf1052 deleted the multiwindow branch Feb 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment