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.

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

[7_4_X] fix(ios): create timer using target and selector instead of block #10438

Closed

Conversation

janvennemann
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-26391

the block solution is only available on iOS 10+. to remain compatible with iOS 8 and 9 move to a target object and selector that will be called when the timer fires.

@sgtcoolguy I opened this as a separate PR because i think we won't need the managed values now that we store the callback and arguments in the target object. JSValue objects will protect their JSValueRef, so as long as we hold a strong reference to them in the target object they won't get GC'ed.

@build
Copy link
Contributor

build commented Nov 7, 2018

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@janvennemann janvennemann force-pushed the timer-compat-fix-7_4_X branch 2 times, most recently from d2073b0 to be7b530 Compare November 8, 2018 00:50
the block solution is only available on iOS 10+. to remain compatible with iOS 8 and 9  move to a target object and selector that the timer will trigger.

related: TIMOB-26391
Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

I think this solution is OK, but I'm not real fond of the duplication/circular references between the TimerManager and the new TimerTarget.

I think we should let the runloop/timer API handle dealing with managing cleanup of timers and the associated data (callbacks, args). I understand what led you here, is that you want to be very good about cleaning up our timer map, but I think there's an easier way to use weak references to the timers.

/**
* Object acting as the target that receives a message when a timer fires.
*/
@interface KrollTimerTarget : NSObject
Copy link
Contributor

Choose a reason for hiding this comment

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

So, looks like you and I are converging together to very similar solutions here. You're right, we don't need to use JSManagedValues, I was wrong about that. That actually ended up causing deadlocks which is why my PRs keep timing out on tests.

Feels to me like a little overkill to hold all of this in a new type. We're already holding the timer id/map in the timer manager. The callback (and also arguments) could be passed in as the "user info".

Single-shot timers will get invalidated automatically after first being fired, so I'm not sure we really need to retain the repeat value in user info. I suppose it means we then are better about cleaning up our map after firing? My "solution" here was to actually use a strong key to weak object map, because the run loop already holds a strong reference to the timer and invalidates/cleans it up once it's done. So in my implementation the weak pointer lets it do that, and really only the NSNumber key is left "alive".

I did simplify and ignore the rare use case of additional arguments, but I suppose I could just pass in the whole argument array as the userInfo and pull the callback args apart when the timer fired.


- (void)timerFired:(NSTimer *)timer
{
[self.callback callWithArguments:self.arguments];
Copy link
Contributor

Choose a reason for hiding this comment

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

one thing that both of us forgot about here was handling exceptions. I suppose if an error/exception gets thrown, the API may just hold onto it in the context and continue on? Should we be registering our exception handler to deal with them? I had just added that locally because I wanted to see what would happen if a timer threw an Error (and it appears to just squash it and move on right now).

@sgtcoolguy
Copy link
Contributor

Closing in favor of #10428

@sgtcoolguy sgtcoolguy closed this Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants