search

redux-saga / redux-saga

An alternative side effect model for Redux apps
https://redux-saga.js.org/
MIT License
22.54k stars 1.97k forks source link

Chrome 129 crash in flush event channel #2698

Open AbeykoonOshan opened 2 months ago

AbeykoonOshan commented 2 months ago

Description of the bug/issue

We encountered a RangeError with an "Invalid array length" while using redux-saga in conjunction with a WebSocket message channel. This issue occurred in our production environment. Based on the error stack trace, the failure is attributed to an array push operation within the flush function in

https://github.com/redux-saga/redux-saga/blob/9ec63efbffb576a5d2c8b4bbf9df8f020693fdad/packages/core/src/internal/buffers.js#L34

const flush = () => { let items = [] while (length) { items.push(take()) } return items }

This issue occurs intermittently and is not consistently reproducible. At times, the error does not occur even when the WebSocket is handling a large volume of data.

This was working perfectly with previous chrome versions. ex: Chrome 128.

The flush function of redux-saga versions between 1.1.3 and 1.3.0 seems to be crashing in Chrome 129.

Environment information

OS: Windows 11 Chrome version: 129.0.6668.59

Andarist commented 2 months ago

If the error starts to happen in Chrome 129 it's something that should be raised in the Chrome's issue tracker

ProChirathF commented 2 months ago

@Andarist can you please advice on what specific details that might help the Chromium team about the issue. As @AbeykoonOshan reported this issue is random and the error is quite generic, a hint around inner workings of the specific code might help in understanding the issue.

AbeykoonOshan commented 6 days ago

Hi @Andarist,

We are again experiencing the same issue with Chrome version 131.0.6778.70. Even in this time the error and the stack trace for the error is same as before.

The error stack trace as follows, RangeError: Invalid array length at Array.push (<anonymous>) at Object.c [as flush] (https://abc.com/site.js:86:59686) at Object.flush (https://abc.com/site.js:86:62844) at En.<computed> (https://abc.com/site.js:86:66437) at https://abc.com/site.js:86:68749 at h (https://abc.com/site.js:86:69660) at d (https://abc.com/site.js:86:69185) at c (https://abc.com/site.js:86:69445)}

We have already raised a ticket with chromium (https://issues.chromium.org/issues/379584347).

In order to get insights of whats happening here we added a try catch block to the failing piece of code.

const flush = () => { try { let items = []; while (length) { items.push(take()); } } catch (error) { alert(Flush error length ${length} -- array ${items && JSON.stringify(items)} --> ${error.stack || error.message}); } }

The length variable value during the crash is 0. This made us think how can the item.push(take()) fail here since the length variable is already 0. Now we think the stack trace we are now seeing somehow is not exposing the root cause for the crash.

Can there be a issue with the minified code. Following is the minified code for the failing code block.

try { for (var e = []; r; ) e.push(a()); return e } catch (error) { alert(Flush error length ${r} -- array ${e && JSON.stringify(e)} --> ${error.stack || error.message}); }

Can you please give us even a hint what can cause such a RangeError for this piece of code block.

Andarist commented 6 days ago

Nothing there should raise such an error. If it does then I'd have to see a repro case that I could investigate. It's likely that it might be a bug in some Chrome version but at that point - we can't easily fix it here. You might have to explore it deeper to look for workarounds, maybe there are some preconditions leading to this bug and maybe u'd be able to avoid them.

AbeykoonOshan commented 4 days ago

We managed to recreate this issue thrice in our selenium scripts. All crashed in the flush function Twice starting length = 60 ending length = 0 array length = 112813858

Once starting length = 2 ending length = 0 array length = 112813858

Obviously causing a "RangeError" since array length is 112813858

Minified flush function

c=function(){
  const valOfR = r; // <---------------- INITIAL VALUE OF "r" IS CAPTURED HERE (Eg: 60)
  try {
    for(var e=[];r;)
      e.push(a()); // <--- THE ITEMS RETURNED FROM FUNCTION "a( )" IS ADDED HERE. THIS ARRAY THROWS THE RANGEERROR. (LENGTH 112813858)
    return e
  } catch (error) {
    error.r = r;[error.ir](http://error.ir/) = valOfR;error.l= e.length;throw error;
  }
}

Minified take function

a = function() {
                if (0 != r) {
                    var t = n[o];
                    return n[o] = null,
                    r--,  // <------------------------ "r" IS REDUCED FOR EVERY "a( )" INVOCATION
                    o = (o + 1) % e,
                    t
                }
            }

In the error that is thrown the loop starts at 60 and reduces to 0 however the array that is created has "112813858" elements where it should only have 60.

The corresponding chromium error https://issues.chromium.org/issues/379584347 If you can give any input that error please add comments, Further are there any unit tests ect. that we can run to further isolate the error.

AbeykoonOshan commented 4 days ago

This is the detail stack trace mapped to redux saga code.

RangeError: Invalid array length

items.push(take())    of: flush 
cb(buffer$1.flush())  of: flush(cb)
channel.flush(cb)     of: runFlushEffect(env, channel, cb)
effectRunner(env, effect.payload, currCb, executingContext)   of: runEffect(effect, effectId, currCb)
finalRunEffect(effect, effectId, currCb);  of: digestEffect(effect, parentEffectId, cb, label)
mainTask.cont(result.value) of: next(arg, isErr)
cb(res, isErr) of: currCb(res, isErr)

We have also tried to create a sample app to recreate the error. https://codesandbox.io/p/sandbox/redux-saga-message-channel-buffer-z5vt9n?file=%2Fsrc%2Findex.js%3A13%2C16

Looking at the stack trace can you advice us on creating a sample app to recreate the problem or is there any unit test that we can use to recreate the problem.

Further our app uses websocket channel and flush effect to get data buffered in the channel.

AbeykoonOshan commented 1 day ago

As per the further investigations we have collected data on the RangeError. Below is the code snippet that was used for capturing the data in the flush and take functions. To capture this data we provided the websocket event channel with a custom ringBuffer (copy of the original ring buffer with additional debug information).

const take = () => {
    if (length != 0) {
      let it = arr[popIndex];
      arr[popIndex] = null;
      length--;
      popIndex = (popIndex + 1) % limit;
      return it;
    }
  };

  const flush = () => {
    let items = [];
    let i = 0;
    const initialLength = length;
    const initialBufferLimit = limit;
    let lastBufItem = null;
    try {
      while (length) {
        const bufItem = take();
        lastBufItem = bufItem;

        const nomatch = items.length !== i;
        if (nomatch) {
          const msg1 = `index=${i} | initLength=${initialLength} | initBufferLimit=${initialBufferLimit}`;
          const msg2 = `| currLength=${length} | currBuffeLimit=${limit} | currArrayLength=${items.length}`;
          console.error(msg1, msg2, ' | array item=', bufItem, items.slice(0, Math.min(500, items.length)));
        }
        i++;
        items.push(bufItem);
      }
      return items;
    } catch (error: any) {
      error.msg1 = `crashedIndex=${i} | initLength=${initialLength} | initBufferLimit=${initialBufferLimit}`;
      error.msg2 = `| crashLength=${length} | crashBuffeLimit=${limit} | crashArrayLength=${items.length}`;
      error.popIndex = popIndex;
      error.pushIndex = pushIndex;
      error.lastBufItm = lastBufItem;
      error.arr = items.slice(0, Math.min(500, items.length));
      error.dsArr = arr;
      console.error('flush', error);
      throw error;

    }

  };

Below is the detail of a captured crash.

{
   initBufferLimit: 100 // THIS IS THE SIZE OF THE BUFFER WHEN flush FUNCTION WAS INVOKED
   initLength: 3 // THE length VARIABLE WAS SET TO 3 WHEN THE flush WAS INVOKED
   crashLength: 0 // WHEN THE RangeError OCCURRED THE length HAS REDUCED TO 0 CALLING take FUNCTION 
   crashBuffeLimit: 100 // AT RangeError THE BUFFER LIMIT REMAIN UNCHANGED
   crashedIndex=112813859 // EVEN THOUGH THE length STARTED AT 3 AND REDUCED TO 0 THE NUMBER OF LOOP EXECUTIONS WERE 112813859, THIS IS THE i VARIABLE
   crashArrayLength=112813858 // THIS IS THE LENGTH OF THE flush ARRAY WHICH HAS 3 VALID ITEMS AND ALL OTHERS ARE NULL
   popIndex:71, // AT THE BOTH POP AND PUSH INDEX REMAINED AS 71, IMPLYING TAKE WAS EXECUTED 3 TIMES AND THE INDICES ARE CORRECT 
   pushIndex:71,
   arr:[ // THIS IS THE NEW ARRAY CREATED USING THE flush FUNCTION AND THE CRASH LENGTH OF THIS WAS 112813859
      {
         type:"SOCKET_DATA",
         payload:"socket-data-payload"
      },
      {
         type:"SOCKET_DATA",
         payload:"socket-data-payload"
      },
      {
         type:"SOCKET_DATA",
         payload:"socket-data-payload"
      },
      null,
      null,
      ................................ // <-- AFTER FIRST 3 VALID ELEMENT EVERYTHING ELSE IS NULL
      null
   ],
   dsArr:[ // THIS IS THE RING BUFFER AFTER POPING ALL ELEMENTS, THIS ARRAY HOLD ALL NULLS AND LENGTH REMAINS CORRECTLY AS 100
      null,
      null,
      ....................
      null
   ]
}

Following is our project tech stack:

So far we haven't succeeded in creating a sample project because it's random. Do you have any hints?.

This error only happens in Chromium based browsers not in Firefox. Chrome also in the latest last 2 version and Chrome 129...59.

Based on the findings one option is to return the flush function when take is null.

 const take = () => {
    if (length != 0) {
      let it = arr[popIndex]
      arr[popIndex] = null
      length--
      popIndex = (popIndex + 1) % limit
      return it
    }
  }

  const flush = () => {
    let items = []
    while (length) { 
      const item =  take();
     if (item == null) return items;
      items.push(item)
    }
    return items
  }

However this is masking the root cause. Thoughts?

AbeykoonOshan commented 21 hours ago

Further, another option would be to exit the loop when pushIndex is matched with the popIndex.

const push = (it) => {
    arr[pushIndex] = it
    pushIndex = (pushIndex + 1) % limit
    length++
}

const take = () => {
    if (length != 0) {
      let it = arr[popIndex]
      arr[popIndex] = null
      length--
      popIndex = (popIndex + 1) % limit
      return it
    }
}

const flush = () => {
    let items = []
    while (true) { 
      if (pushIndex === popIndex) return items; // exit the loop when push and pop indices are same
      const item =  take();
      items.push(item)
    }
    return items
  }

Any Thoughts?