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.

Sign up for GitHub

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

Fix reloading when using useAnimatedKeyboard #3932

Merged
merged 4 commits into from
Feb 10, 2023
Merged

Fix reloading when using useAnimatedKeyboard #3932

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82984ca
Fix reloading when using useAnimatedKeyboard
graszka22 Jan 11, 2023
154d2d4
Observer cleanup
graszka22 Jan 16, 2023
ae18750
Merge branch 'main' into graszka22/fix-keyboard-reload
graszka22 Feb 2, 2023
02a2fff
Fix second reload
graszka22 Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 27 additions & 5 deletions ios/keyboardObserver/REAKeyboardEventObserver.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ - (instancetype)init
_listeners = [[NSMutableDictionary alloc] init];
_nextListenerId = @0;
_state = UNKNOWN;

NSNotificationCenter *notificationCenter = [NSNotificationCenter defaultCenter];

[notificationCenter addObserver:self
selector:@selector(clearListeners)
name:RCTBridgeDidInvalidateModulesNotification
object:nil];

return self;
}

Expand Down Expand Up @@ -79,8 +87,10 @@ - (void)unsubscribeFromKeyboardEvents:(int)listenerId

- (void)runAnimation
{
displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(updateKeyboardFrame)];
[displayLink addToRunLoop:[NSRunLoop mainRunLoop] forMode:NSRunLoopCommonModes];
if (!displayLink) {
displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(updateKeyboardFrame)];
[displayLink addToRunLoop:[NSRunLoop mainRunLoop] forMode:NSRunLoopCommonModes];
}
}

- (void)stopAnimation
Expand Down Expand Up @@ -173,9 +183,12 @@ - (int)subscribeForKeyboardEvents:(KeyboardEventListenerBlock)listener
- (void)unsubscribeFromKeyboardEvents:(int)listenerId
{
NSNumber *_listenerId = [NSNumber numberWithInt:listenerId];
[_listeners removeObjectForKey:_listenerId];
if ([_listeners count] == 0) {
[[NSNotificationCenter defaultCenter] removeObserver:self];
[self->_listeners removeObjectForKey:_listenerId];
if ([self->_listeners count] == 0) {
[[NSNotificationCenter defaultCenter] removeObserver:self name:UIKeyboardWillHideNotification object:nil];
[[NSNotificationCenter defaultCenter] removeObserver:self name:UIKeyboardWillShowNotification object:nil];
[[NSNotificationCenter defaultCenter] removeObserver:self name:UIKeyboardDidHideNotification object:nil];
[[NSNotificationCenter defaultCenter] removeObserver:self name:UIKeyboardDidShowNotification object:nil];
}
}

Expand All @@ -193,6 +206,15 @@ - (void)recognizeInitialKeyboardState
});
}

- (void)clearListeners
{
RCTUnsafeExecuteOnMainQueueSync(^() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this function may lead to deadlocks in some cases, can't we just run this block asynchronously?

Copy link
Contributor

@jwajgelt jwajgelt Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want listeners to be run after clearListeners has been called, that's the root of the issue here.
If you call it asynchronously, after scheduling this block the js runtime will be cleaned up, and if the listeners run before this block is executed, you get a crash.
That said, there may be better way of doing this, like having an atomic boolean invalidated which stops the listeners from being called - might be worth trying out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of other ways to do this.
The problem with boolean is that at some point before calling the listeners I we have to check if the boolean is set. Something like this:

if(!invalidated) {
   for listener in listeners
      call listener
}

and now what if we invalidate js runtime while calling the listeners? It is still going to crash.
We can try checking the boolean before calling every listener like this:

for listener in listeners
   if(!invalidated)
      call listener

I didn't test it but I think there is still a couple of instructions between checking the boolean and actually calling a listener so theoretically it can still crash.

I think there is no way around the fact that js thread has to wait somehow for the main thread to finish calling listeners.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check would have to somehow be done on the JS thread, this way you won't be able to invalidate the runtime while running the listener, since operations on the same thread won't be interleaved. That said, if these listeners run on the main thread, why do they use the JS thread runtime?
Or am I misunderstanding and the UI thread runtime is destroyed on the JS thread for some reason?

Copy link
Member Author

@graszka22 graszka22 Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the xcode debugger it is destroyed on the JS thread. You can even see it on the second screenshot.
Or is it the js runtime destroyed... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the UI thread runtime is destroyed on the JS thread and called on the main thread and we rather can't change that. So I have no other idea than synchronising threads like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say, the current approach is ok.
I don't think we expect the reload to come from the main thread, so there shouldn't be any deadlocks here.

Actually, does this listener actually run on the JS thread? I'm not sure where NSNotificationCenter listeners run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Regular notification centers deliver notifications on the thread in which the notification was posted. [...] At times, you may require notifications to be delivered on a particular thread that is determined by you instead of the notification center. [...] In these cases, you must capture the notifications as they are delivered on the default thread and redirect them to the appropriate thread."

So I guess they are run from JS thread then 👍.

[self->_listeners removeAllObjects];
[self->displayLink invalidate];
self->displayLink = nil;
});
}

jwajgelt marked this conversation as resolved.
Show resolved Hide resolved
#endif

@end