primevue icon indicating copy to clipboard operation
primevue copied to clipboard

Toast: Race condition on remove

Open zavog opened this issue 1 year ago • 3 comments

Describe the bug

When removing a toast and adding one or more toasts at the same time, the wrong toast may be removed.

The following is the current code in Toast.vue. When a toast is added after index = i and before splice, then the wrong toast is removed, which unfortunately might not even match the condition that was passed to remove.

remove(params) {
    let index = -1;

    for (let i = 0; i < this.messages.length; i++) {
        if (this.messages[i] === params.message) {
            index = i;
            break;
        }
    }

    this.messages.splice(index, 1);
    this.$emit(params.type, { message: params.message });
}

Reproducer

https://stackblitz.com/edit/primevue-create-vue-typescript-issue-template-gkirw2?file=src%2FApp.vue

PrimeVue version

3.48.0

Vue version

3.x

Language

TypeScript

Build / Runtime

Vite

Browser(s)

No response

Steps to reproduce the behavior

Click the "Trigger" button in the Stackblitz

Expected behavior

The remove function should never remove a task that doesn't match the condition

zavog avatar Feb 06 '24 15:02 zavog

Yup, it only removes the last toast added regardless of the identifier you provide.

ZiadJ avatar Feb 08 '24 16:02 ZiadJ

Thanks a lot for your report! I set a milestone for it. We'll check it before the milestone is released.

mertsincan avatar Feb 09 '24 10:02 mertsincan

This is not a race condition.

Root Cause in Toast.vue

  • The remove method tries to find message in the messages array with object equality. (if (this.messages[i] === params.message))
  • But all stored message objects are converted into proxy objects when a new toast is added, because this.messages is reactive. (this.messages = [...this.messages, message])
  • This causes the object equality condition to fail.
  • In this case the found index is -1 and splice will remove the last item. (this.messages.splice(index, 1))

Possible Fix

Use message.id for the equality test because the stored objects are not guaranteed to be the same instance: if (this.messages[i].id === params.message.id)

This will result in the intended behaviour (as if it was object equality) because the id's are unique.

Temporary Workaround

I created a wrapper around useToast which always converts the passed message object into a reactive proxy object. With this workaround the correct toast is removed.

marknn3 avatar Feb 22 '24 02:02 marknn3

This is not a race condition.

Root Cause in Toast.vue

  • The remove method tries to find message in the messages array with object equality. (if (this.messages[i] === params.message))
  • But all stored message objects are converted into proxy objects when a new toast is added, because this.messages is reactive. (this.messages = [...this.messages, message])
  • This causes the object equality condition to fail.
  • In this case the found index is -1 and splice will remove the last item. (this.messages.splice(index, 1))

Possible Fix

Use message.id for the equality test because the stored objects are not guaranteed to be the same instance: if (this.messages[i].id === params.message.id)

This will result in the intended behaviour (as if it was object equality) because the id's are unique.

Temporary Workaround

I created a wrapper around useToast which always converts the passed message object into a reactive proxy object. With this workaround the correct toast is removed.

In my project I ended up patching with this updated remove function: `remove(params) { const index = this.messages.findIndex((m) => m.id === params.message.id);

        if (index !== -1) {
            this.messages.splice(index, 1);
            this.$emit(params.type, { message: params.message });
        }
    },

`

dragomirweb avatar Mar 20 '24 08:03 dragomirweb