Page MenuHomePhabricator

Consider Locking Validators When `validateStatus` Is Called
Closed, ResolvedPublic

Description

function-schemata code can be found at https://gerrit.wikimedia.org/g/mediawiki/services/function-schemata

ajv validators store errors on the validator function itself. Our wrapper code (javascript/src/schema.js; look for this task's ID) generates a ValidationStatus object more-or-less like this:

booleanResult = validateFunction( something);
status = validationStatus( booleanResult, validateFunction.errors );

Conceivably, if the same validator were called twice simultaneously, validateFunction.errors could be overwritten before any status was generated:

Thread1: booleanResult = validateFunction( something );
Thread2: booleanResult = validateFunction( something );
Thread1: # tries to generate status with Thread2's errors!

Do something about this:

  • we can allocate a new validator for each validation event (expensive)
  • we can put a mutex inside the validateStatus code (JavaScript)

Event Timeline

Change 758544 had a related patch set uploaded (by Cory Massaro; author: Cory Massaro):

[mediawiki/services/function-schemata@master] BREAKING CHANGE: protect AJV validation behind a mutex to avoid race conditions when propagating errors.

https://gerrit.wikimedia.org/r/758544

Change 758544 merged by jenkins-bot:

[mediawiki/services/function-schemata@master] [BREAKING CHANGE] Protect AJV validation behind a mutex to avoid race conditions when propagating errors

https://gerrit.wikimedia.org/r/758544

Change 759519 had a related patch set uploaded (by Cory Massaro; author: Cory Massaro):

[mediawiki/services/function-orchestrator@master] Update function-schemata sub-module to HEAD (81742d0)

https://gerrit.wikimedia.org/r/759519

Change 759519 merged by jenkins-bot:

[mediawiki/services/function-orchestrator@master] Update function-schemata sub-module to HEAD (81742d0)

https://gerrit.wikimedia.org/r/759519

Change 760578 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/WikiLambda@master] Update function-schemata sub-module to HEAD (220f67a)

https://gerrit.wikimedia.org/r/760578

Change 760578 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] Update function-schemata sub-module to HEAD (220f67a)

https://gerrit.wikimedia.org/r/760578

Change 761476 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/services/function-evaluator@master] Update function-schemata sub-module to HEAD (0d127c2)

https://gerrit.wikimedia.org/r/761476

Change 761476 merged by jenkins-bot:

[mediawiki/services/function-evaluator@master] Update function-schemata sub-module to HEAD (0d127c2)

https://gerrit.wikimedia.org/r/761476

Sorry for commenting on such an old ticket, but I was reading on some AJV docs and they seem to indicate the race condition would not happen in our scenario: https://ajv.js.org/faq.html#would-errors-get-overwritten-in-case-of-concurrent-validations. As long as we read the errors in the same execution block as the validate call, which I believe we are/can, they won't be overwritten. Here is a discussion thread about this: https://github.com/ajv-validator/ajv/issues/242. Here is the part of the code where the mutex is used: https://github.com/wikimedia/mediawiki-services-function-schemata/blob/master/javascript/src/schema.js#L472-L473.

Perhaps y'all already went through all of these and I am just totally repeating a historic discussion here. Just thought any chance to de-async the validator functions is worth trying :D

After talking with @cmassaro we decided to re-open this task and undo the mutex+async. Reusing this task for the undo!

Sorry for commenting on such an old ticket, but I was reading on some AJV docs and they seem to indicate the race condition would not happen in our scenario: https://ajv.js.org/faq.html#would-errors-get-overwritten-in-case-of-concurrent-validations. As long as we read the errors in the same execution block as the validate call, which I believe we are/can, they won't be overwritten. Here is a discussion thread about this: https://github.com/ajv-validator/ajv/issues/242. Here is the part of the code where the mutex is used: https://github.com/wikimedia/mediawiki-services-function-schemata/blob/master/javascript/src/schema.js#L472-L473.

Perhaps y'all already went through all of these and I am just totally repeating a historic discussion here. Just thought any chance to de-async the validator functions is worth trying :D

Million thanks! Yes, wipe away my mutex sins.

Change 823186 had a related patch set uploaded (by Mary Yang; author: Mary Yang):

[mediawiki/services/function-schemata@master] Remove mutex locks and async references in function schemata.

https://gerrit.wikimedia.org/r/823186

Change 824808 had a related patch set uploaded (by Mary Yang; author: Mary Yang):

[mediawiki/services/function-orchestrator@master] Propagate the no-async validator changes to orchestrator.

https://gerrit.wikimedia.org/r/824808

Change 823186 merged by jenkins-bot:

[mediawiki/services/function-schemata@master] Remove mutex locks and async references in function schemata.

https://gerrit.wikimedia.org/r/823186

Change 831927 had a related patch set uploaded (by Cory Massaro; author: Cory Massaro):

[mediawiki/services/function-orchestrator@master] Update function-schemata sub-module to HEAD (f601b78)

https://gerrit.wikimedia.org/r/831927

Change 831927 merged by jenkins-bot:

[mediawiki/services/function-orchestrator@master] Update function-schemata sub-module to HEAD (f601b78)

https://gerrit.wikimedia.org/r/831927

Change 824808 merged by jenkins-bot:

[mediawiki/services/function-orchestrator@master] Propagate the no-async validator changes to orchestrator.

https://gerrit.wikimedia.org/r/824808

Change 833353 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/WikiLambda@master] Update function-schemata sub-module to HEAD (6cf755c)

https://gerrit.wikimedia.org/r/833353

Change 833355 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/services/function-evaluator@master] Update function-schemata sub-module to HEAD (6cf755c)

https://gerrit.wikimedia.org/r/833355

Change 833356 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/tools/wikilambda-cli@master] Update function-schemata sub-module to HEAD (6cf755c)

https://gerrit.wikimedia.org/r/833356

Change 833356 merged by jenkins-bot:

[mediawiki/tools/wikilambda-cli@master] Update function-schemata sub-module to HEAD (6cf755c)

https://gerrit.wikimedia.org/r/833356

Change 833355 merged by jenkins-bot:

[mediawiki/services/function-evaluator@master] Update function-schemata sub-module to HEAD (6cf755c)

https://gerrit.wikimedia.org/r/833355

Change 833353 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] Update function-schemata sub-module to HEAD (6cf755c)

https://gerrit.wikimedia.org/r/833353