Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Race condition when using ngMessages with $asyncValidators #12856

Closed
jamlee1 opened this issue Sep 15, 2015 · 10 comments
Closed

Race condition when using ngMessages with $asyncValidators #12856

jamlee1 opened this issue Sep 15, 2015 · 10 comments

Comments

@jamlee1
Copy link

jamlee1 commented Sep 15, 2015

The issue is that $asyncValidators will set the validity to undefined while it fulfills its promise, which will cause the error to briefly go away. At this point, ngMessage's render method will process the $errors (which will be {}), and will detach the messageCtrl. But before that is complete, the async validator finishes and sets the error back to true, and ngMessage's render method is run. Then $destroy handler is invoked and ngMessage will deregister the message node, causing the message to go away and never come back.

See the error in action:

https://gist.github.com/anonymous/1bd3f9b6cb17498e73c0

@topherfangio
Copy link

One of our Material users is experiencing a similar issue (angular/material#4685) that I believe is related to this. Basically, typing very quickly in the input causes the errors to disappear.

If you edit the Plunkr in the above issue to use ngMessages 1.3.15 (or 1.3.19), the issue disappears and it reappears if you use 1.4.0.

You can also test this directly on the Material Site's input demo by scrolling down to the "Errors" demo and very quickly deleting/typing in the "Hourly Rate (USD)" field a few times: The $... an hour? That's a little ridiculous. I doubt event Bill Clinton could afford that. message will appear and then disappear and never come back.

Please let me know if I can help debug or provide more info.

@petebacondarwin Thomas asked me to ping you an alert on this as the Angular Material community is seeing this in our Input Demo

@willisd2
Copy link

@jamlee1 @topherfangio @petebacondarwin I'm experiencing this same behavior in an application of mine. I've tried to create a spike to demonstrate the issue but have been unsuccessful. I've got it narrowed down to some particular areas, and believe it is an issue with ngAnimate moreso than ngMessages. In my app when I remove ngAnimate as a module for the application, this behavior goes away.

In my scenario I have an asyncValidator on a form input that hits the server to check for the existence of a particular number that is entered in. This is the behavior I see:

Page loads up and the form field is valid since nothing is entered into it. If I enter in a number that is invalid, the correct validation message appears via ngMessages. If I delete the number and enter in another invalid one, the message disappears and reappears with no issues. If I go from an invalid number to a valid one and back to an invalid one, everything is fine. If an invalid number is currently entered in and the validation message is displayed and I add another digit to the number (also making it an invalid number) for a split second I see a duplicate validation message appear, and then they both disappear never to return again.

When debugging the last scenario mentioned above, I can see the following happen:
While waiting for the asyncValidator to return a response, detach is called on the ngMessages controller to remove the currently displayed error message. Removal of the message gets routed through ngAnimate. Before ngAnimate fully removes the message from the DOM, the asyncValidator returns and tells the ngMessages controller to attach the validation message to the DOM. The message gets attached to the DOM (the old one still hasn't been removed, and 2 messages are displayed at once). Finally, the '$destroy' event is called on the original validation message, and 'detach' is called on the ngMessages controller. Since the duplicate validation message is really a clone of the original one, 'detach' removes both messages from the DOM, never to return again.

As I said, I haven't been able to replicate this outside of my app. However, when stepping through the code of the spike I made using Chrome, I can sometimes get it to happen, only when stepping through though. It appears to be a timing issue with how the animations are handled (even though I don't have any animations associated with these validation messages).

@petebacondarwin
Copy link
Contributor

I'll take a look over the weekend or on Monday.

@Narretz
Copy link
Contributor

Narretz commented Sep 19, 2015

@petebacondarwin
Copy link
Contributor

@willisd2 said:

When debugging the last scenario mentioned above, I can see the following happen:
While waiting for the asyncValidator to return a response, detach is called on the ngMessages controller to remove the currently displayed error message. Removal of the message gets routed through ngAnimate. Before ngAnimate fully removes the message from the DOM, the asyncValidator returns and tells the ngMessages controller to attach the validation message to the DOM. The message gets attached to the DOM (the old one still hasn't been removed, and 2 messages are displayed at once). Finally, the '$destroy' event is called on the original validation message, and 'detach' is called on the ngMessages controller. Since the duplicate validation message is really a clone of the original one, 'detach' removes both messages from the DOM, never to return again.

This is spot on. Looking for a fix...

@petebacondarwin
Copy link
Contributor

By the way, a quick workaround is to put an ng-if on your ng-messages to remove the messages element when it is valid: http://codepen.io/anon/pen/GpqWGp

@petebacondarwin
Copy link
Contributor

So the current sequence of things when typing a second character is:

    detach       -- detach error message as we are temporarily valid (waiting for async timeout)
    attach       -- attach error message as the timeout has completed as invalid
    $destroy     -- ngAnimate removes the previously detached message DOM node
    detach       -- removal of the destroyed DOM node triggers a detach on the new error message, since we reused the `message` object
    $destroy     -- the detach of the error message triggers its associated DOM node to be removed, triggering another `$destroy`

What we want to happen is, either wait to attach a new message only after the previous message's node's animation has completed:

    detach       -- detach error message as we are temporarily valid (waiting for async timeout)
    $destroy     -- ngAnimate removes the detached message DOM node
    attach       -- attach error message as the timeout has completed as invalid

or to use a new message object when the previous message is still waiting to be destroyed, so that the new message is not affected by an async destruction of the node:

    detach       -- detach error message as we are temporarily valid (waiting for async timeout)
    attach       -- attach error message, using a new independent message object, as the timeout has completed as invalid
    $destroy     -- ngAnimate removes the detached message DOM node

or to cancel the leave animation if the message is re-attached before the animation has completed. I am not sure if this is possible yet in ngAnimate.

@petebacondarwin
Copy link
Contributor

I think that this is the basis of a fix: angular:master...petebacondarwin:issue-12856

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Sep 21, 2015
If `ngMessage` tried to add a message back in that was about to be removed
after an animation, the NgMessageController got confused and tried to detach
the newly added message, when the pending node was destroyed.

This change applies a unique `attachId` to the message object and its DOM
node when it is attached. This is then checked when a DOM node is being
destroyed to prevent unwanted calls to `detach`.

Closes angular#12856
@petebacondarwin
Copy link
Contributor

I have a fix here: #12903

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Sep 22, 2015
If `ngMessage` tried to add a message back in that was about to be removed
after an animation, the NgMessageController got confused and tried to detach
the newly added message, when the pending node was destroyed.

This change applies a unique `attachId` to the message object and its DOM
node when it is attached. This is then checked when a DOM node is being
destroyed to prevent unwanted calls to `detach`.

Closes angular#12856
Closes angular#12903
@ghost
Copy link

ghost commented Sep 22, 2015

Don't mail me
On Sep 22, 2015 10:29 PM, "Pete Bacon Darwin" notifications@github.com
wrote:

Closed #12856 #12856 via
#12903 #12903.


Reply to this email directly or view it on GitHub
#12856 (comment).

DISCLAIMER

Any views or opinions presented in this email are solely those of the
author and do not necessarily represent those of the Ganpat University.

DISCLAIMER

Any views or opinions presented in this email are solely those of the
author and do not necessarily represent those of the Ganpat University.

petebacondarwin added a commit that referenced this issue Sep 22, 2015
If `ngMessage` tried to add a message back in that was about to be removed
after an animation, the NgMessageController got confused and tried to detach
the newly added message, when the pending node was destroyed.

This change applies a unique `attachId` to the message object and its DOM
node when it is attached. This is then checked when a DOM node is being
destroyed to prevent unwanted calls to `detach`.

Closes #12856
Closes #12903
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.