Potential Race Condition

lonnylot commented 4 years ago

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"}}

SimonErm commented 4 years ago

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.

lonnylot commented 4 years ago

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.. :/

lonnylot commented 4 years ago

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

lonnylot commented 4 years ago

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).

SimonErm commented 4 years ago

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?

lonnylot commented 4 years ago

Yeah, I think using a recursive setTimeout is probably the best approach. My only concern is if it causes any memory leaks - I can’t quite recall in what situations JS holds onto/releases memory.

Sent from my iPhone

On Dec 13, 2019, at 7:13 PM, SimonErm notifications@github.com wrote:

i wrote this test:

it.skip('test', 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);
        }
    };
    done();
});
which works properly(only when awaiting the promise), so i think this should be the way to go. What do you think?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

lonnylot commented 4 years ago

@SimonErm Let me know your thoughts on this: https://github.com/SimonErm/react-native-job-queue/compare/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.

SimonErm commented 4 years ago

Looks good to me, but is the clearInstance necessary? Yes I started a mock implementation locally and will add it the next days.

lonnylot commented 4 years ago

@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.

SimonErm commented 4 years ago

@lonnylot I'm sorry for the late response, but i was busy with exams. I did the changes on a fix branch. Can you test #10 ?

SimonErm commented 4 years ago

Fixed in version 0.1.0

SimonErm / react-native-job-queue

Potential Race Condition #8