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

[iOS] Timing: Fixes timer when app get into background #24649

Closed
wants to merge 4 commits into from

Conversation

zhongwuzw
Copy link
Contributor

Summary

Related #23674, in that PR, we imported background timer support, but it's not sufficient, I think the reason that works is because it enable the Background Modes and do some background tasks, for the users who don't enable it, timer would pause immediately before goes into background.

To fix it, we can mark a background task when goes into background, it can keep app active for minutes, try best to support timing when in background.

cc. @cpojer .

Changelog

[iOS] [Fixed] - Timing: Fixes timer when app get into background

Test Plan

Run code in background like the code below, console can log correctly.

    setInterval(() => {
      console.log("Timer run");
    }, 1000);

@zhongwuzw zhongwuzw requested a review from shergin as a code owner April 29, 2019 13:55
@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Apr 29, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 29, 2019
@jacobp100
Copy link
Contributor

jacobp100 commented May 1, 2019

Just a note that react-intl will set huge timeouts (sometimes days) for its <FormattedRelative> components, so this could cause huge issues for some people's code

@zhongwuzw
Copy link
Contributor Author

@jacobp100 Eh, what do you mean? can you expand which issues you want to express? iOS can't run for days in background.

@jacobp100
Copy link
Contributor

I just wanted to highlight a popular library people use on React Native calls setTimeout with a delay that can last for days. If we wait for every set timeout by running the app in the background, it could, in some cases, be a drain on the battery.

Not saying it shouldn't be merged, I just wanted to highlight one potential issue!

@zhongwuzw
Copy link
Contributor Author

@jacobp100 I don't know wether I get which issue you want to express. You mean it may has battery issue? Actually, we already support background timer in #23674(Many people have expressed a desire for background timer support on iOS ), and background task can only last most a few minutes, so we only can support background timer for a short time, if app not return to foreground, it will be killed by system.
And if you have some other issues, please comment, and we'll try to fix them. Thanks.

@jacobp100
Copy link
Contributor

jacobp100 commented May 1, 2019

Sorry, thought this was adding the support. You're right, already there. Ignore me! 😄

@zhongwuzw
Copy link
Contributor Author

Fixed #25083.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Let's ship it. Thank you for the fix.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @zhongwuzw in 3382984.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 30, 2019
@craftzdog
Copy link

Cheers!

@bharatidudhrejiya
Copy link

I have updated RCTTiming.m file with lasted code but still js code not work when app in background.

thanks!

radko93 added a commit to radko93/react-native that referenced this pull request Oct 30, 2019
radko93 added a commit that referenced this pull request Oct 31, 2019
radko93 added a commit that referenced this pull request Oct 31, 2019
radko93 added a commit to radko93/react-native that referenced this pull request Oct 31, 2019
esamelson added a commit to expo/react-native that referenced this pull request Nov 1, 2019
grabbou pushed a commit that referenced this pull request Nov 4, 2019
facebook-github-bot pushed a commit that referenced this pull request Nov 5, 2019
…27073)

Summary:
This PR reverts commit 3382984 that is causing #26696 #26995.
> app would be closed immediately after going to background on iOS 13.1/13.2 and was investigated by minhtc #26696 (comment). The commit that is being reverted is apparently causing the app to be closed immediately. This has to be reverted in master separately as the file differs there.

Similar PR for 0.61. branch #27065

## Changelog

[iOS] [Fixed] - Fix apps crashing on iOS 13.x when running timer in the background
Pull Request resolved: #27073

Test Plan: Try [this](3382984#commitcomment-35745287) snippet on iOS 13.1/13.2, the app should not crash anymore

Differential Revision: D18323679

Pulled By: cpojer

fbshipit-source-id: 3af7036a0e1d3811924e581c649b16e5a4667e83
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
Related facebook#23674, in that PR, we imported background timer support, but it's not sufficient, I think the reason that works is because it enable the `Background Modes` and do some background tasks, for the users who don't enable it, timer would pause immediately before goes into background.

To fix it, we can mark a background task when goes into background, it can keep app active for minutes, try best to support timing when in background.

cc. cpojer .

## Changelog

[iOS] [Fixed] - Timing: Fixes timer when app get into background
Pull Request resolved: facebook#24649

Differential Revision: D15554451

Pulled By: cpojer

fbshipit-source-id: a33f7afe6b63d1a4fefcb7098459aee0c09145da
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants