react-native-audio-waveform icon indicating copy to clipboard operation
react-native-audio-waveform copied to clipboard

Android : fix multiple leaking promises + bonuses

Open dprevost-LMI opened this issue 1 year ago • 2 comments
trafficstars

The Android code has multiple places without a guarantee of resolving the promise. This PR provides numerous fixes for issues I encountered while playing audio in the list. Most of them are just simple code review refactoring.

For example, just opening one audio file made three promise hangs, and each call to seekToPlayer leaks. So, just clicking 500 times on the waveform triggers the below error:

Excessive number of pending callbacks: 501. Some pending callbacks that might have leaked by never being called from native code: {"55":{"module":"AudioWaveform","method":"setPlaybackSpeed"},"89":{"module":"AudioWaveform","method":"setPlaybackSpeed"}

Highlights:

  • Keeping promises inside AudioWaveformModule helps to ensure promises are resolved correctly
  • Moving promises out of AudioPlayer for a leaner class and not "react-native" tainted
  • Doing try-catch so the app does not crash when having an exception

Bonus:

  • I added a feature in the example to list recorded audio after the app reboot!
  • I removed the deprecation of onPlayerStateChanged in favour of onPlaybackStateChanged
  • I reviewed the weird handling of onError and simplified it
  • I removed a few Promise.resolve and Promise.reject combined with try-catch that are useless and can be simplified by returning the value directly or throwing an error since the method is already async
  • I fixed a case where the promise leaks when the path in the preparePlayer is wrong. If we play a file with an invalid path, the player never gets ready and gets stuck idle. Since player.prepare() should move it out of idle, if it stays in idle, there is a problem, and the promise is rejected.
  • I set props of IStartPlayer mandatory else; I test it, and passing no value crashes the android, so there are no optional
  • I removed the await preparePlayerForPath(); inside the onDidFinishPlayingAudio since this one is circular, where you trigger the player's end but recreate it immediately. Also, after removal, it still works
  • Code simplification by using standard functions to validate the player key string and the audio player in the hash map

Let me know what you think!

Note: I may need to remove my logger logPromise tracking the leaking promises before you merge!

dprevost-LMI avatar Oct 05 '24 21:10 dprevost-LMI