Only call sync_api when necessary

[sssoleileraaa]

Description

sync_api is called every 5 minutes and any time the refresh icon is clicked.

It should not be called when a star is successfully updated or when a source is successfully deleted. If those calls are successful, there is nothing to do client-side other than log and update the message in the activity status bar.

It would make more sense to call sync_api before (1) a source is deleted, (2) a source is starred, or (3) a reply is sent. This will be helpful when updating the UI to reflect what is current on the server in a way where we don't redraw everything or have to rearrange widgets too often. Here are some examples that show why this is useful:

Example 1

Journalist A deleted a source that Journalist B is currently starring.

If the sync is enqueued first, the client would remove the source from local storage, and then the star job would come back with an error that says the source no longer exists.

Example 2

Journalist sends a message and its stuck in the queue for a while because of network delays or issues

If the sync is enqueued first, the client would be able to grab the latest messages and display them in the UI so that when the reply is sent the (local only) grayed out pending reply widget would be deleted and the new reply widget would be append to the conversation to match the server.

Alternative idea

I just want to mention this alternative idea. Local changes are pushed to the server without calling sync_api at all. This would mean that things are more likely to get rearranged on the next sync cycle. @redshiftzero proposed that we have metadata syncs more often, like every 15 seconds, to avoid this. This works fine too, and might be a better solution. Or perhaps it makes sense to do both.

@redshiftzero thoughts?

(Also, in the future, journalists and sources may be able to delete their own messages. And journalists may even be able to delete source messages. These actions would update the server and affect what both journalists and sources see when looking at the conversation. This is just something to make mental note of for now.)

[sssoleileraaa]

Resolves Related to #463

[eloquence]

How would these proposals impact perceived performance from the user's point of view when starring or deleting a source, or sending a reply? Does the performance impact depend on the number of submissions on the server?

[redshiftzero]

Local changes are pushed to the server without calling sync_api at all.

I think is a good idea. We should:

• Never use sync_api to update the UI. We should update the UI elements directly using a signal to the relevant widget.
• Revisit decide on synchronization strategy when pushing to servers #51 for actions that have slow feedback due to calling controller.sync_api to sidestep the issue of "what happens if the action fails and the state awkwardly reverts".

One of the reasons the client UI can seem sluggish right now (putting aside the network instability, see #643) is the controller.sync_api calls that are happening still a fair bit in the codebase.

For example, right now the following happens:

1. User clicks star.
2. Queue is doing lots of stuff OR tor is very slow so the star seems to take a very long time to update.
3. Star finally updates after a weirdly long delay.

The good news is that for many operations we do represent the change immediately with a pending state in the UI such that the user has feedback immediately. For sending replies we update the UI immediately indicating that a reply is in a pending state. For message and reply downloads we have a pending state when we haven't fetched/decrypted the content. For sync we set the sync icon to active so the user knows things are happening. For file downloads we plan to implement a progress indicator.

Areas where supported actions can be sluggish:

• source starring - this is an action we can keep applying as many times as we want until we learn it succeeds since it is idempotent (just noticed we use POST and not PUT on this endpoint, woops). This may be one reason to add the persistent queue to resend these idempotent operations until they succeed even after the user logs back in. The user never knows there was an issue as they see their state change locally immediately and eventually it will be consistent with the server when they reconnect to the network.
• deletion of sources - this is an action where we should add an indicator indicating that it can fail (the failure will pause the queue). This is also why we originally discussed having this be blocking, but that's imho pretty annoying for users. See Show deletion progress indicator in UI #643.

[sssoleileraaa]

I think it's important for the UI to be responsive to user actions. Even with intermediate states that show the user we're working on it, we should try to complete the action as soon as possible. Performing an additional sync really only makes sense before sending a reply in my opinion. This is because we only sync every 5 minutes and a message could come in during that time period. Syncing before the reply gets downloaded and decrypted after it's saved server-side makes it so we only have to redraw the conversation view once after the reply is sent. This is an edge case, and perhaps i'm thinking too much in terms of fast back-and-forth conversations like we have in slack, signal, github (sometimes), etc. But really there's very little reason to not sync before finishing a SendReplyJob since we already immediately show a reply even before it has been saved on the server.

Otherwise, I'm in agreement with not bothering with a sync for deletion or staring.

Update (for clarity)

Overall I think what we're trying to avoid is clogging the queue with the highest priority job, MetadataSyncJob, so that the client continues to be responsive since we don't yet support concurrency in the main queue. For delete, we just need to delete the source locally if delete_source was successful serverside; we don't need to do a sync (no more jobs need to be submitted to the queue). For staring, we don't need to do a sync if the star job is successful (also we update the GUI immediately so really nothing needs to be done unless it fails, in which case we need to undo the toggle, rather than call sync_api). For replies I think we need to sync after the server has been updated successfully because we want to make sure we show any new messages that might have come in before or during the SendReplyJob.

[eloquence]

But really there's very little reason to not sync before finishing a SendReplyJob since we already immediately show a reply even before it has been saved on the server.

Well, right now we're showing a reply as "pending" with the translucent background, and it will later show as "failed" if the user now closes the client. IMO it is crucial to complete the action as soon as possible, especially if it only takes 2-3 seconds to complete in the typical case. The likelihood that this 2-3 seconds is when a sync would show a new reply seems exceedingly small. Am I misunderstanding/missing something?

[sssoleileraaa]

The likelihood that this 2-3 seconds is when a sync would show a new reply seems exceedingly small. Am I misunderstanding/missing something?

Only that a message could be sent by a source before the message even begins sending, which is more like a 5-minute window rather than a 2-3 second window. If we want to redraw the Converstion View after decrypting a reply then I am saying that it makes sense to also do a sync to see if we need to draw any new message bubbles too.

[sssoleileraaa]

ah, related: #653 (comment)

[eloquence]

Only that a message could be sent by a source before the message even begins sending, which is more like a 5-minute window

Is it? As I understand it:

• Large file downloads are handled in their own thread.
• We're aiming to prioritize user-initiated actions (Deprioritize background jobs over user-initiated actions #657) over background jobs.
• User-initiated actions other than downloads all should complete quickly - i.e. in the 2.5 second per action case for the typical case

Is that correct? If so, under what circumstances would the user have to wait 5 minutes before a reply starts sending?

[sssoleileraaa]

Is it? As I understand it:

• Large file downloads are handled in their own thread.
• We're aiming to prioritize user-initiated actions (Deprioritize background jobs over user-initiated actions #657) over background jobs.
• User-initiated actions other than downloads all should complete quickly - i.e. in the 2.5 second per action case for the typical case

2.5 seconds for a refresh has not been what we've experienced though, e.g. #610, which happened during syncs, including user-initiated syncs, which makes the same server requests (get_all_replies get_submisions get_sources). This issue was resolved by increasing the timeouts to 20 seconds instead of 5 seconds. Here are all the api endpoints that need benchmarking: #648

Is that correct? If so, under what circumstances would the user have to wait 5 minutes before a reply starts sending?

Ah, this might be the misunderstanding. I was saying that an automated sync happens every 5 minutes, so if a message was sent at T minus 4.5 minutes, and then a reply was sent at T minus 4 minutes, we would have to wait 4 minutes until the message shows up in the gui, unless we sync with the server before we refresh the gui. I'm saying we should prioritize the SendReplyJob, but we should also kick off a MetadataSyncJob onSuccess before updating the Conversation View.

[eloquence]

Ah, this might be the misunderstanding. I was saying that an automated sync happens every 5 minutes, so if a message was sent at T minus 4.5 minutes, and then a reply was sent at T minus 4 minutes, we would have to wait 4 minutes until the message shows up in the gui, unless we sync with the server before we refresh the gui. I'm saying we should prioritize the SendReplyJob, but we should also kick off a MetadataSyncJob onSuccess before updating the Conversation View.

I think our priority should be to visibly transition the reply from "sending" to "sent" as quickly as the job queue and the Tor network allows, so the user gets immediate visible feedback that the operation was successful. If we want to add syncs (full or partial) after that operation (including the GUI update) we can, just in case the source interacts with a journalist closer to real-time. Does that make sense?

[redshiftzero]

I think user expectations are that the UI will update much more often than every 5 minutes, it's kind of weird that it takes so long. Regarding:

I'm saying we should prioritize the SendReplyJob, but we should also kick off a MetadataSyncJob onSuccess

I think this is a workaround for the fact that we want to be syncing much more often to make sure changes show up quickly... What about we implement #652 as a separate network health queue that does much more frequent syncing (this way the user-triggered actions occur concurrently with the metadata syncing)?

[sssoleileraaa]

Moving to a separate queue (a queue that never pauses to allow network pings) would unclog the main queue. Before doing this I think there are couple smaller issues that'll help keep the client responsive:

• Sync icon should be disabled during active sync #388
• No longer sync on successful delete or star operation #658
• For replies, I agree that it's syncing before updating the gui is a workaround for a slow 5-minute sync cycle and more frequent syncs would be ideal, which means needing support for async jobs in the main queue or a separate queue.

[eloquence]

@creviera I think we have a clear set of next steps here now, which should also be roughly reflected in the backlog on our board. Are you OK with closing this issue?

[sssoleileraaa]

Closing since we've broken this up into smaller issues and are managing this on the project board now