Race condition in _pluralizeAliases

[seanrucker]

https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/serializer.js#L1107-L1131

I presume the first line of this method:

if (this._didPluralizeAliases) { return; }

...is to avoid calling this method more than once. The problem is that the _didPluralizeAliases flag is not set to true until the end of the method. Normally that wouldn't be a problem, but from what I can tell the Ember.assert method executes asynchronously. This creates a race condition where the _pluralizeAliases method can be called again before the _didPluralizeAssets flag is set to true.

Seems the simple fix is to set the flag right after the check:

if (this._didPluralizeAliases) { return; }
this._didPluralizeAliases = true;

It looks like there are a few places where this technique is being used and could potentially have the same race conditions.

[bobjackman]

Seems like the more robust solution here would be to use some sort of synchronous assert or other form of error checking rather than prematurely marking these various operations complete.

[wagenet]

Ember.assert is definitely not async. However, it does seem fine to set the flag right after the conditional. Feel free to submit a separate PR to address this.

[bobjackman]

... it does seem fine to set the flag right after the conditional ...

When setting the flag immediately, consider the following: If the calling function were to implement some type of try/catch block when calling this method, and some exception were encountered, the calling function would catch it and attempt to recover. If it cared to check, _didPluralizeAliases would already be set to true when, in fact, aliases have not been pluralized due to an error.

Since there is currently no such error handling structure in place, I guess it comes down to personal preference here, but setting the flag prematurely will definitely add to Ember's technical debt.

[wagenet]

@kogi This method is not very large and is self-contained. I think if we decided to add that, it would be obvious enough.