gdelataillade / alarm

A Flutter plugin to easily manage alarms on iOS and Android
https://pub.dev/packages/alarm
MIT License
132 stars 86 forks source link

fix(iOS): SwiftAlarmPlugin.scheduleAppRefresh() EXC_BAD_ACCESS #161

Closed longtn-imt closed 8 months ago

longtn-imt commented 8 months ago
Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Subtype: UNKNOWN_0xD at 0x0000000000000000
Exception Codes: 0x000000000000000d, 0x0000000000000000

Thread 0::  Dispatch queue: com.apple.main-thread
0   libicucore.A.dylib      0x7fff214c30a2 0x7fff2139a000 + 1216674
...
19  BackgroundTasks         0x7fff4b980a4a -[BGTaskScheduler submitTaskRequest:error:] + 139
20  alarm                   0x10a201585 specialized static SwiftAlarmPlugin.scheduleAppRefresh() + 309 (SwiftAlarmPlugin.swift:459)
21  alarm                   0x10a1fb1c6 $s5alarm16SwiftAlarmPluginC18scheduleAppRefreshyyFZ + 5 [inlined]
22  alarm                   0x10a1fb1c6 closure #2 in closure #2 in SwiftAlarmPlugin.setAlarm(call:result:) + 166 (SwiftAlarmPlugin.swift:151)
23  alarm                   0x10a1f9369 thunk for @escaping @callee_guaranteed () -> () + 25
...
gdelataillade commented 8 months ago

Hi @longtn-imt

Thanks a lot for sharing your work.

Looks good to me. Could you confirm if all your tests passed successfully? I plan to run some additional tests on my end to ensure everything is in order before proceeding with the merge.

gdelataillade commented 8 months ago

Hi @longtn-imt

I just added some changes in the main branch that creating conflicts with your PR. Should be very easy to resolve though.

longtn-imt commented 8 months ago

Hi @gdelataillade I am testing 64 alarms, each alarm is separated by 10 minutes. (iOS maximum 64 schedule notification)

Can you make the schedule notification optional? I try https://pub.dev/packages/flutter_local_notifications Push notification with payload and show alarm screen using notification launch app.

gdelataillade commented 8 months ago

Hi @longtn-imt

Sorry I can't make the notification optional because for Android it's mandatory to show a notification when the alarm foreground service is started. It is how Android is designed.

About this PR, could you justify a little bit your 2 changes about the scheduleAppRefresh moved and the .broadcast() added on the ringStream ?

longtn-imt commented 8 months ago

https://stackoverflow.com/questions/51396769/flutter-bad-state-stream-has-already-been-listened-to When I logout and login, I can't listen to the stream

gdelataillade commented 8 months ago

Ok when what about the scheduleAppRefresh ? Your PR title suggests it fixes a EXC_BAD_ACCESS error.

longtn-imt commented 8 months ago

Flutter use Main.thread handle logic UI Sometimes when i run application error exception EXC_BAD_ACCESS You should test multiple alarms (I testing ~64 alarms)

gdelataillade commented 8 months ago

OK, I'm merging this PR.

Thank you very much for your work.

gdelataillade commented 8 months ago

Hi @longtn-imt

Upon further review, I've decided to reverse the modification that transitioned ringStream to a broadcast stream, and I wanted to explain my reasoning clearly:

The nature of broadcast streams means that any listeners subscribed to the stream will only start receiving events added to the stream from the point of their subscription onward. They will miss any events that were emitted prior to this. This behavior poses a significant challenge for some use cases, especially in scenarios where the app needs to handle ringStream immediately upon launching after being killed.

My approach involves checking for any active alarms as soon as the app launches, and if an alarm is found to be ringing, I add this event to the stream. The critical issue arises if the stream doesn't have any listeners at that moment, which is a plausible scenario since this check is performed in the main function. Without an active listener, the crucial information about the alarm ringing is effectively lost, which undermines the reliability of the alarm functionality.

Given this constraint, maintaining ringStream as a non-broadcast stream is essential to ensure that the alarm alert can be reliably captured and handled, even if the app is launched in response to an alarm event. However, I value your suggestion and will explore potential solutions to address the stream issue in future updates, ensuring I do so without introducing any breaking changes to the current version.