Potential Race Condition
lonnylot opened this issue · 11 comments
I keep getting warnings similar to below. I am thinking that there is a race condition here where it takes longer than updateInterval to call getNextJob.
Is there a reason this wasn't implemented similar to how it was done in react-native-queue Queue.ts?
ExceptionsManager.js:126 Warning: Please report: Excessive number of pending callbacks: 501. Some pending callbacks that might have leaked by never being called from native code: {"1615":{"module":"RNPermissions","method":"requestNotifications"},"2630":{},"2631":{},"2632":{},"2633":{"module":"UIManager","method":"measureInWindow"},"2634":{"module":"UIManager","method":"measureInWindow"},"2635":{"module":"UIManager","method":"measureInWindow"},"2636":{"module":"UIManager","method":"measureInWindow"},"2651":{"module":"RNPermissions","method":"check"},"2654":{"module":"JobQueue","method":"getNextJob"},"2655":{"module":"JobQueue","method":"getJobsForWorker"},"2656":{"module":"JobQueue","method":"getJobsForWorker"},"2657":{"module":"JobQueue","method":"getJobsForWorker"},"2658":{"module":"JobQueue","method":"getJobsForWorker"},"2659":{"module":"JobQueue","method":"getJobsForWorker"},"2660":{},"2661":{},"2662":{},"2663":{},"2664":{},"2671":{"module":"Networking","method":"sendRequest"},"2673":{"module":"JobQueue","method":"getNextJob"},"2674":{"module":"JobQueue","method":"getNextJob"},"2692":{"module":"JobQueue","method":"getNextJob"},"2693":{"module":"UIManager","method":"measureInWindow"},"2694":{"module":"UIManager","method":"measureInWindow"},"2695":{"module":"UIManager","method":"measureInWindow"},"2696":{"module":"UIManager","method":"measureInWindow"},"2697":{"module":"UIManager","method":"measureInWindow"},"2698":{"module":"JobQueue","method":"getNextJob"},"2699":{"module":"UIManager","method":"measureInWindow"},"2700":{"module":"UIManager","method":"measureInWindow"},"2717":{"module":"UIManager","method":"measureInWindow"},"2865":{},"2872":{},"2873":{},"2874":{},"2875":{},"2876":{},"2877":{},"2878":{},"2879":{},"2880":{},"2881":{},"2882":{},"2883":{},"2884":{},"2885":{},"2886":{},"2887":{},"2888":{},"2889":{},"2890":{},"2891":{},"2892":{},"2893":{},"2894":{},"2895":{},"2896":{},"2897":{},"2898":{},"2899":{},"2900":{},"2901":{},"2902":{},"2903":{},"2904":{},"2905":{},"2906":{},"2907":{},"2908":{},"2909":{},"2910":{},"2911":{},"2912":{},"2913":{},"2914":{},"2915":{},"2916":{},"2917":{},"2918":{},"2919":{},"2920":{},"2921":{},"2922":{},"2923":{},"2924":{},"2925":{},"2926":{},"2927":{},"2928":{},"2929":{},"2930":{},"2931":{},"2932":{},"2933":{},"2934":{},"2935":{},"2936":{},"2937":{},"2938":{},"2939":{},"2940":{},"2941":{},"2942":{},"2943":{},"2944":{},"2945":{},"2946":{},"2947":{},"2948":{},"2949":{},"2950":{},"2951":{},"2952":{},"2953":{},"2954":{},"2955":{},"2956":{},"2957":{},"2958":{},"2959":{},"2960":{},"2961":{},"2962":{},"2963":{},"2964":{},"2965":{},"2966":{},"2967":{},"2968":{},"2969":{},"2970":{},"2971":{},"2972":{},"2973":{},"2974":{},"2975":{},"2976":{},"2977":{},"2978":{},"2979":{},"2980":{},"2981":{},"2982":{},"2983":{},"2984":{},"2985":{},"2986":{},"2987":{},"2988":{},"2989":{},"2990":{},"2991":{},"2992":{},"2993":{},"2994":{},"2995":{},"2996":{},"2997":{},"2998":{},"2999":{},"3000":{},"3001":{},"3002":{},"3003":{},"3004":{},"3005":{},"3006":{},"3007":{},"3008":{},"3009":{},"3010":{},"3011":{},"3012":{},"3013":{},"3014":{},"3015":{},"3016":{},"3017":{},"3018":{},"3019":{},"3020":{},"3021":{},"3022":{},"3023":{},"3024":{},"3025":{},"3026":{},"3027":{},"3028":{},"3029":{},"3030":{},"3031":{},"3032":{},"3033":{},"3034":{},"3035":{},"3036":{},"3037":{},"3038":{},"3039":{},"3040":{},"3041":{},"3042":{},"3043":{},"3044":{},"3045":{},"3046":{},"3047":{},"3048":{},"3049":{},"3050":{},"3051":{},"3052":{},"3053":{},"3054":{},"3055":{},"3056":{},"3057":{},"3058":{},"3059":{},"3060":{},"3061":{},"3062":{},"3063":{},"3064":{},"3065":{},"3066":{},"3067":{},"3068":{},"3069":{},"3070":{},"3071":{},"3072":{},"3073":{},"3074":{},"3075":{},"3076":{},"3077":{},"3078":{},"3079":{},"3080":{},"3081":{},"3082":{},"3083":{},"3084":{},"3085":{},"3086":{},"3087":{},"3088":{},"3089":{},"3090":{},"3091":{},"3092":{},"3093":{},"3094":{},"3095":{},"3096":{},"3097":{},"3098":{},"3099":{},"3100":{},"3101":{},"3102":{},"3103":{},"3104":{},"3105":{},"3106":{},"3107":{},"3108":{},"3109":{},"3110":{},"3111":{},"3112":{},"3113":{},"3114":{},"3115":{},"3116":{},"3117":{},"3118":{},"3119":{},"3120":{},"3121":{},"3122":{},"3123":{},"3124":{},"3125":{},"3126":{},"3127":{},"3128":{},"3129":{},"3130":{},"3131":{},"3132":{},"3133":{},"3134":{},"3135":{},"3136":{},"3137":{},"3138":{},"3139":{},"3140":{},"3141":{},"3142":{},"3143":{},"3144":{},"3145":{},"3146":{},"3147":{},"3148":{},"3149":{},"3150":{},"3151":{},"3152":{},"3153":{},"3154":{},"3155":{},"3156":{},"3157":{},"3158":{},"3159":{},"3160":{},"3161":{},"3162":{},"3163":{},"3164":{},"3165":{},"3166":{},"3167":{},"3168":{},"3169":{},"3170":{},"3171":{},"3172":{},"3173":{},"3174":{},"3175":{},"3176":{},"3177":{},"3178":{},"3179":{},"3180":{},"3181":{},"3182":{},"3183":{},"3184":{},"3185":{},"3186":{},"3187":{},"3188":{},"3189":{},"3190":{},"3191":{},"3192":{},"3193":{},"3194":{},"3195":{},"3196":{},"3197":{},"3198":{},"3199":{},"3200":{},"3201":{},"3202":{},"3203":{},"3204":{},"3205":{},"3206":{},"3207":{},"3208":{},"3209":{},"3210":{},"3211":{},"3212":{},"3213":{},"3214":{},"3215":{},"3216":{},"3217":{},"3218":{},"3219":{},"3220":{},"3221":{},"3222":{},"3223":{},"3224":{},"3225":{},"3226":{},"3227":{},"3228":{},"3229":{},"3230":{},"3231":{},"3232":{},"3233":{},"3234":{},"3235":{},"3236":{},"3237":{},"3238":{},"3239":{},"3240":{},"3241":{},"3242":{},"3243":{},"3244":{},"3245":{},"3246":{},"3247":{},"3248":{},"3249":{},"3250":{},"3251":{},"3252":{},"3253":{},"3254":{},"3255":{},"3256":{},"3257":{},"3258":{},"3259":{},"3260":{},"3261":{},"3262":{},"3263":{},"3264":{},"3265":{},"3266":{},"3267":{},"3268":{},"3269":{},"3270":{},"3271":{},"3272":{},"3273":{},"3274":{},"3275":{},"3276":{},"3277":{},"3278":{},"3279":{},"3280":{},"3281":{},"3282":{},"3283":{},"3284":{},"3285":{},"3286":{},"3287":{},"3288":{},"3289":{},"3290":{},"3291":{},"3292":{},"3293":{},"3294":{},"3295":{},"3296":{},"3297":{},"3298":{},"3299":{},"3300":{},"3301":{},"3302":{},"3303":{},"3304":{},"3305":{},"3306":{},"3307":{},"3308":{"module":"JobQueue","method":"getNextJob"},"3309":{"module":"JobQueue","method":"getNextJob"},"3310":{"module":"JobQueue","method":"getNextJob"},"3311":{"module":"JobQueue","method":"getNextJob"},"3312":{"module":"JobQueue","method":"getNextJob"},"3313":{"module":"JobQueue","method":"getNextJob"},"3314":{"module":"JobQueue","method":"getNextJob"},"3315":{"module":"JobQueue","method":"getNextJob"},"3316":{"module":"JobQueue","method":"getNextJob"},"3317":{"module":"JobQueue","method":"getNextJob"},"3318":{"module":"JobQueue","method":"getNextJob"},"3319":{"module":"JobQueue","method":"getNextJob"},"3320":{"module":"JobQueue","method":"getNextJob"},"3321":{"module":"JobQueue","method":"getNextJob"},"3322":{"module":"JobQueue","method":"getNextJob"},"3323":{"module":"JobQueue","method":"getNextJob"},"3324":{"module":"JobQueue","method":"getNextJob"},"3325":{"module":"JobQueue","method":"getNextJob"},"3326":{"module":"JobQueue","method":"getNextJob"},"3327":{"module":"JobQueue","method":"getNextJob"},"3328":{"module":"JobQueue","method":"getNextJob"},"3329":{"module":"JobQueue","method":"getNextJob"},"3330":{"module":"JobQueue","method":"getNextJob"},"3331":{"module":"JobQueue","method":"getNextJob"},"3332":{"module":"JobQueue","method":"getNextJob"},"3333":{"module":"JobQueue","method":"getNextJob"},"3334":{"module":"JobQueue","method":"getNextJob"},"3335":{"module":"JobQueue","method":"getNextJob"},"3336":{"module":"JobQueue","method":"getNextJob"},"3337":{"module":"JobQueue","method":"getNextJob"},"3338":{"module":"JobQueue","method":"getNextJob"}}
can you provide some code to reproduce this?
I decided to use setIntervall over a while loop since the while loop has a huge cpu load compared to setIntervall.
can you provide some code to reproduce this?
I don't have a simple demo nailed down because my code isn't public and it's a race condition so it's difficult to duplicate. It's happening when the app is busy doing a lot of other things.
In this instance it's on first load after someone signs in and the screen navigates to the dashboard and starts requesting permissions.
I decided to use setIntervall over a while loop since the while loop has a huge cpu load compared to setIntervall.
That's what I was thinking. I wasn't sure if the same issue existed on React Native.
But it is clear from the code that the runQueue function will be called whether or not the previous call finished. Any thoughts on adding something like a lock? Something like:
private runQueue = async () => {
if (this.queueRunningLock) {
return;
}
this.queueRunningLock = true;
const nextJob = await this.jobStore.getNextJob();
if (!this.isActive) {
this.finishQueue();
}
if (this.isJobNotEmpty(nextJob)) {
const nextJobs = await this.getJobsForWorker(nextJob.workerName);
const processingJobs = nextJobs.map(this.excuteJob);
Promise.all(processingJobs);
} else if (!this.isExecuting()) {
this.finishQueue();
}
this.queueRunningLock = false;
};It still has a bit of a race condition, but should prevent multiple runs.. :/
Another thought is to use a while loop, but force it onto a separate thread: https://facebook.github.io/react-native/docs/native-modules-ios#threading
And a last thought, and then I'll try to implement something is to add a while loop and a lifetime value. That way if you find it causing issues you can manage the length of the while loop from outside the package (and especially if you need things to process as background tasks).
i wrote this test:
it('test setInterval', async (done) => {
let i = 0;
const intervalFunc = async () => {
console.log(i);
if (i % 2) {
await new Promise((resolve) => setTimeout(() => resolve(i++), 200));
} else {
await new Promise((resolve) => setTimeout(() => resolve(i++), 400));
}
};
const interval = setInterval(intervalFunc, 10);
await new Promise((resolve) => setTimeout(resolve, 2000));
clearInterval(interval);
done();
});to reproduce the race condition with setInterval and you're right they occure with the async functions even with awaiting the promise. I also tested a recursiv setTimeout looking like this:
it('test setTimeout', async (done) => {
let i = 0;
const intervalFunc = async () => {
console.log(i);
if (i % 2) {
await new Promise((resolve) => setTimeout(() => resolve(i++), 200));
} else {
await new Promise((resolve) => setTimeout(() => resolve(i++), 400));
}
if (i < 100) { //the conditon would be the isExecuting
setTimeout(intervalFunc, 10);
}
};
setTimeout(intervalFunc, 10);
await new Promise((resolve) => setTimeout(resolve, 2000));
done();
});which works properly(only when awaiting the promise), so i think this should be the way to go.
What do you think?
@SimonErm Let me know your thoughts on this: master...lonnylot:issue/remove-queue-race
I think that is the direction you were heading and I am pretty sure it avoids any unintentional memory leaks. The tests can still be improved, but the previous iteration didn't run at all so I figure it's a good starting point w/ room for improvement later.
Looks good to me, but is the clearInstance necessary? Yes I started a mock implementation locally and will add it the next days.
@SimonErm The 'clearInstance' was so I could get the jest test to work. Without it there was an error thrown when adding the testWorker in the second test.
Fixed in version 0.1.0