Refactor stale promise consumption #1348

matthewrfennell · 2024-12-28T23:48:59Z

This WIP PR contains refactors to allow an error popup on consumption of stale, rejected promises.

See matthewrfennell@3d8c5d2 for how I was thinking of implementing that error posting (based off this branch).

Feedback welcome, I've left as WIP as there are still some things I want to check (left comments for myself to review later)

This is so we can differentiate between fulfilled and rejected promises when retrieving them from the DB.

This will be used to iterate over all promises in the DB on app startup, showing a notification for any ones that were rejected.

This is used to keep track of the information we need to show an error banner to the user if needed.

This is because, when we remove a stale promise from the DB, we are effectively consuming it (i.e. removing it / marking it as not needed anymore).

Previously, we had special handling for stale promises in consumeStalePromises. That function manually reached into the DB and removed them directly. However, removing stale promises is really a special case of consuming them - in both cases the intent is "this promise is no longer needed and can be removed from everywhere we keep track of it" - the only difference between stale and non-stale promises is that it is not possible to perform the UI action in the stale case (although you may want to perform some other action, such as showing an error popup). Therefore: generalise attemptConsume to handle both stale and non-stale promises - only calling the AnyPromise if the promise is stale, but removing it from the DB in either case. As an extra bonus, remove the removeAllPromises: method which was only used here.

matthewrfennell · 2024-12-28T23:59:47Z

Monal/Classes/MLPromise.m


 @property(nonatomic, strong) AnyPromise* anyPromise;
 @property(nonatomic, strong) id resolvedArgument;
-@property(nonatomic, assign) BOOL isResolved;
+@property(nonatomic, strong) MLPromiseRejection* rejection;


It doesn't feel very clean to have a dedicated MLPromiseRejection that will be nil in the vast majority of cases - consider creating a union to cleanly separate the fulfilled and rejected cases.

Yes, a union or tagged enum would be a good fit here :)

matthewrfennell · 2024-12-29T00:00:37Z

Monal/Classes/MLPromise.m

@@ -136,26 +192,29 @@ -(void) attemptConsume
        return;
    }

-    if(!self.isResolved)
+    if(self.state == PromiseResolutionStateUnresolved)


We previously deleted all stale promises from the DB, but now they will only be deleted if they are resolved. I need to think if this is the right behaviour or not.

Problem is: this could still lead to stale promises: promises never resolved or rejected (for example all iq handler that don't have a matching invalidation handler that rejects the promise).

There are multiple solutions to this:

Always implicitly pass a promise to the handler (maybe named _promise) and create a generic invalidation handler to be specified if no special invalidation is to be used. That generic invalidation would reject the promise given in the _promise paramter. The promise passed in could be auto-generated with some macro magic if no var named _promise is in scope when the $newHandler macro is used.

Just silently remove unresolved promises from the db on app start without displaying an error. This way you'd have to write an explicit invalidation handler if you want to display an error on invalidation. No macro magic needed.

Case 2 has a downside, though: IQs that don't get resolved (or time out after 30 seconds, which automatically generates a fake IQ error response) before the XEP-0198 smacks session times out and a completely new xmpp session is created, won't generate any error. Even if the main app isn't killed (that'll leave the ui in a "spinning" state forever).
That downside even is a unresolved problem in our current codebase: The promise would neither be resolved nor rejected in that case. We'd have

I think solution 2 is better nonetheless.
We could fix this by:

generate fake error IQs in the smacks error case, too.

write invalidation handlers accompanying all iq handlers in our codebase (that are a lot)

automatically call a generic invalidation handler if no invalidation handler was specified and that generic invalidatiuon handler could iterate over all handler arguments, pick out all MLPromises and reject them with some generic error message. That's possible, because calling an invalidation handler makes sure, the normal handler will never called in the future and thus all MLPromises are free to be resolved without any conflict (if not already resolved of course).

I prefer solution 3.

maybe I prefer solution 1 (together with 3?).

matthewrfennell · 2024-12-29T00:02:31Z

Monal/Classes/MLPromise.h

+
+@end
+
+@interface MLPromiseRejection : NSObject<NSSecureCoding>


I don't like exposing this in the header, but it's necessary to add the entry in HelperTools unserializeData:

Couldn't you just forward declare this with @class MLPromiseRejection; in HelperTools.m?

It isn't enough in this case unfortunately - I get this compile error in HelperTools.m

Receiver 'MLPromiseRejection' for class message is a forward declaration

Okay, then you could just add

@interface MLPromiseRejection : NSObject<NSSecureCoding> @end

to the header and

@interface MLPromiseRejection() : NSObject<NSSecureCoding> [...] @end

containing all your actual property/method declarations to your MLPromise.m. That way you'll only expose the fact that this class exists, but none of its (internal) methods :)

tmolitor-stud-tu

Added lots of comments :)

tmolitor-stud-tu · 2024-12-29T03:26:00Z

Monal/Classes/MLPromise.h

+
+@end
+
+@interface MLPromiseRejection : NSObject<NSSecureCoding>


Couldn't you just forward declare this with @class MLPromiseRejection; in HelperTools.m?

tmolitor-stud-tu · 2024-12-29T03:28:49Z

Monal/Classes/MLPromise.m

+    PromiseResolutionStateRejected,
+};
+
+@interface MLPromiseRejection()


I'd add a readonly computed property returning the xmpp object for the stored accountID. Similar to the one in MLCall.m.

tmolitor-stud-tu · 2024-12-29T04:00:42Z

Monal/Classes/MLPromise.m


 @property(nonatomic, strong) AnyPromise* anyPromise;
 @property(nonatomic, strong) id resolvedArgument;
-@property(nonatomic, assign) BOOL isResolved;
+@property(nonatomic, strong) MLPromiseRejection* rejection;


Yes, a union or tagged enum would be a good fit here :)

tmolitor-stud-tu · 2024-12-29T04:04:50Z

Monal/Classes/MLPromise.m

 }

-(void) reject:(NSError*) error
+-(void) rejectWithError:(NSError*) error andNode:(XMPPStanza* _Nullable) node forAccountWithID:(NSNumber*) accountID


I think we should provide both a simple -(void) reject:(NSError*) error method and a more complicated -(void) rejectWithError:(NSError*) error andNode:(XMPPStanza* _Nullable) node forAccountWithID:(NSNumber*) accountID

The simple method could just set the accountID to nil or @0 and the HelperTools method would have to be modifed to use something like "Error" or "General Error" as title instead of the jid, if the accountID wan't provided.

tmolitor-stud-tu · 2024-12-29T04:23:34Z

Monal/Classes/MLPromise.m

@@ -136,26 +192,29 @@ -(void) attemptConsume
        return;
    }

-    if(!self.isResolved)
+    if(self.state == PromiseResolutionStateUnresolved)


Problem is: this could still lead to stale promises: promises never resolved or rejected (for example all iq handler that don't have a matching invalidation handler that rejects the promise).

There are multiple solutions to this:

Always implicitly pass a promise to the handler (maybe named _promise) and create a generic invalidation handler to be specified if no special invalidation is to be used. That generic invalidation would reject the promise given in the _promise paramter. The promise passed in could be auto-generated with some macro magic if no var named _promise is in scope when the $newHandler macro is used.

Just silently remove unresolved promises from the db on app start without displaying an error. This way you'd have to write an explicit invalidation handler if you want to display an error on invalidation. No macro magic needed.

Case 2 has a downside, though: IQs that don't get resolved (or time out after 30 seconds, which automatically generates a fake IQ error response) before the XEP-0198 smacks session times out and a completely new xmpp session is created, won't generate any error. Even if the main app isn't killed (that'll leave the ui in a "spinning" state forever).
That downside even is a unresolved problem in our current codebase: The promise would neither be resolved nor rejected in that case. We'd have

I think solution 2 is better nonetheless.
We could fix this by:

generate fake error IQs in the smacks error case, too.

write invalidation handlers accompanying all iq handlers in our codebase (that are a lot)

automatically call a generic invalidation handler if no invalidation handler was specified and that generic invalidatiuon handler could iterate over all handler arguments, pick out all MLPromises and reject them with some generic error message. That's possible, because calling an invalidation handler makes sure, the normal handler will never called in the future and thus all MLPromises are free to be resolved without any conflict (if not already resolved of course).

I prefer solution 3.

tmolitor-stud-tu · 2024-12-29T04:24:43Z

Monal/Classes/MLPromise.m

+        [_resolvers removeObjectForKey:self.uuid];
+        DDLogVerbose(@"Removed resolver with uuid %@ from resolvers map", self.uuid);
+        DDLogVerbose(@"Resolvers map is now: %@", _resolvers);
+    }

    [[DataLayer sharedInstance] removePromise:self];


I talked about removing all promises from the db (even stale ones) in some comment above and now realize, that already happens :)

tmolitor-stud-tu · 2024-12-29T04:28:12Z

Monal/Classes/MonalAppDelegate.m

@@ -395,7 +395,7 @@ -(BOOL) application:(UIApplication*) application willFinishLaunchingWithOptions:
    });

    // Remove stale promises left in the DB that weren't consumed last time we ran the app
-    [MLPromise removeStalePromises];
+    [MLPromise consumeStalePromises];


I think we'll have to extract a list/set of all pending stale promises after making sure the appex is not running anymore (the last act in the will launch method), but consume these stale promises only after creating all xmpp objects later in the app startup cycle.

We must extract all stale promises that early in the app startup cycle to not introduce race conditions with the startup process that could introduce new MLPromises that aren't stale before or while all xmpp objects are created.

I'd not wait for lissine and implement it in such a way, that all accountIDs, for which no enabled account (and thus an xmpp object) could be found, simply show a generic error title instead of the jid (see some comment above). That can simply improved to always show the jid later on after lissine's rewrite.

But even then not giving an xmpp object and using a generic notification title for the error notification in this case would be a useful feature I'd like to have independent of our MLPromise framework.

matthewrfennell added 7 commits December 28, 2024 23:45

Remove unneeded helper function declaration

13f9992

Replace MLPromise isResolved bool with enum

361ca12

This is so we can differentiate between fulfilled and rejected promises when retrieving them from the DB.

Add DataLayer method to get all promises

4c72af2

This will be used to iterate over all promises in the DB on app startup, showing a notification for any ones that were rejected.

Ensure that we do not resolve a promise twice

7f58611

Add MLPromiseRejection class

7b99fd0

This is used to keep track of the information we need to show an error banner to the user if needed.

Rename removeStalePromises - consumeStalePromises

d8b6be9

This is because, when we remove a stale promise from the DB, we are effectively consuming it (i.e. removing it / marking it as not needed anymore).

matthewrfennell marked this pull request as draft December 28, 2024 23:57

matthewrfennell commented Dec 29, 2024

View reviewed changes

tmolitor-stud-tu requested changes Dec 29, 2024

View reviewed changes

tmolitor-stud-tu force-pushed the develop branch 20 times, most recently from b9b6827 to 418c3b9 Compare January 3, 2025 19:00

tmolitor-stud-tu force-pushed the develop branch 9 times, most recently from bae7dea to e32cbe3 Compare January 4, 2025 22:39

tmolitor-stud-tu force-pushed the develop branch 3 times, most recently from fefe9af to b78a732 Compare February 10, 2025 05:31

tmolitor-stud-tu force-pushed the develop branch from 5a8ccb2 to 8bf1d29 Compare February 22, 2025 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor stale promise consumption #1348

Refactor stale promise consumption #1348

matthewrfennell commented Dec 28, 2024 •

edited

Loading

matthewrfennell Dec 28, 2024

tmolitor-stud-tu Dec 29, 2024

matthewrfennell Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024

tmolitor-stud-tu Feb 10, 2025

matthewrfennell Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024

matthewrfennell Dec 29, 2024

tmolitor-stud-tu Dec 31, 2024

tmolitor-stud-tu left a comment

tmolitor-stud-tu Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024

tmolitor-stud-tu Dec 29, 2024


		@end

		@interface MLPromiseRejection : NSObject<NSSecureCoding>

Refactor stale promise consumption #1348

Are you sure you want to change the base?

Refactor stale promise consumption #1348

Conversation

matthewrfennell commented Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmolitor-stud-tu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewrfennell commented Dec 28, 2024 •

edited

Loading