cht-core icon indicating copy to clipboard operation
cht-core copied to clipboard

Race condition in db-object-widget

Open dianabarsan opened this issue 2 years ago • 2 comments

Describe the bug Multiple feedback docs reported this error:

Uncaught (in promise): TypeError: Cannot read properties of undefined (reading 'model')\nTypeError: Cannot read properties of undefined (reading 'model')\n    at https://<host>/main.js?_sw-precache=<hash>:1:2031598\n    at Array.forEach (<anonymous>)\n    at b (https://<host>/main.js?_sw-precache=<hash>:1:2031466)\n    at https://<host>/main.js?_sw-precache=<hash>:1:2031563\n    at Array.forEach (<anonymous>)\n    at b (https://<host>/main.js?_sw-precache=<hash>:1:2031466)\n    at https://<host>/main.js?_sw-precache=<hash>:1:2031563\n    at Array.forEach (<anonymous>)\n    at b (https://<host>/main.js?_sw-precache=<hash>:1:2031466)\n    at HTMLSelectElement.E (https://<host>/main.js?_sw-precache=<hash>:1:2031388)"

This points to: https://github.com/medic/cht-core/blob/52605ac752bb40ab61ee1198fd7fe1bc0fbef982/webapp/src/js/enketo/widgets/db-object-widget.js#L114

Urls where the feedback docs were logged:

  • https:///#/contacts/73fd7622-24da-465f-b77e-088730add0d4
  • https:///#/messages
  • https:///#/contacts

None of these pages should contain xml forms, this makes me think there's a race condition, where a promise resolves after the form was already unloaded.

To Reproduce Steps to reproduce the behavior: TBD

Expected behavior No errors, we should support unloading of forms gracefully, even if there are pending promises.

Environment

  • Instance: live instance
  • Browser: webview?
  • Client platform: Android
  • App: webapp
  • Version: 3.14.x

dianabarsan avatar Apr 27 '22 10:04 dianabarsan

@jkuester Do you think this may be resolved in your db object widget refactor?

garethbowen avatar Jul 25 '22 05:07 garethbowen

@garethbowen no, I do not believe this has been fixed by the Enketo uplift. I was able to recreate this plausibly on both the Enekto branch and master. The db-object-widget will trigger the reference to the form model if the select2 element changes. However, the select2 service will try to hydrate the contact's doc and then trigger a change on the element. So, there is a race condition because if the user closes the form before the doc has been hydrated, then select2 will still try to set it on the element even though the form model is gone.

jkuester avatar Jul 25 '22 22:07 jkuester

Ready for AT in 7602-race-condition.

Reproduction:

  1. Open dev console
  2. Load a form with a db-object-widget select field
  3. Search for and select an option from the widget
  4. Very quickly, quit the form (eg: navigate away). You should have to very quickly answer "yes" to the dirty form modal.

Expected: No error in the console, no feedback doc, no error on the screen. Actual: Error in the console, and feedback doc generated.

garethbowen avatar Feb 16 '23 01:02 garethbowen

Hi @garethbowen, I have not been able to reproduce the error, not sure if I am missing something, could you please help me?

This is what I am trying to do:

Video attached

video

tatilepizs avatar Feb 21 '23 14:02 tatilepizs

I managed to reproduce it with breakpoints and custom JS to make things happen in the right order. Looking at the video, you would need to click much quicker to get it to error, for example, when selecting the family in the dropdown, have your mouse directly over the cancel button so you can just double click, and then hit "enter" to accept the dialog without having to move your mouse.

Another way to buy yourself more time would be to modify the form to bind lots of deeply nested patient info. This gives the code more work to do while you're clicking off the form.

However, I think this might have to be one of those situations where it's too challenging to get the race condition to fire just right, and as long as the new code doesn't break anything else we may need to just go for it...

garethbowen avatar Feb 21 '23 17:02 garethbowen

Thank you! I followed your suggestions but I am still not able to replicate the error, so I just did a smoke test creating patients and different reports using the branch 7602-race-condition and everything is working as expected. So I think that we are good and we can just merge it 🎉

tatilepizs avatar Feb 22 '23 15:02 tatilepizs

Merged!

garethbowen avatar Feb 22 '23 20:02 garethbowen