naja-js / naja

Modern AJAX library for Nette Framework
https://naja.js.org
MIT License
109 stars 15 forks source link

Race hazard in submitForm call #364

Closed janturon closed 1 year ago

janturon commented 1 year ago

Bug Report

Naja 2.4.0, UIHandler.ts:127 (and the clickForm above) should not be declared async. It should return a Promise from makeRequest synchronously (there is no await in the method body). Async call lead to race hazard. In our case where Naja is bundled with other scripts (so the processing is a bit delayed), well defined input to submitForm is undefined in the UIHandler, as the submitForm is called in another thread due to the async. The issue appeared on Safari in minified code only.

There can be other thread safety issues: I suggest to checkout scenarios where Naja is not called in the main thread.

Code that reproduces the problem

N/A (race hazard in bundled code)

jiripudil commented 1 year ago

Hi, I'm afraid I don't fully understand.

The methods are marked async so that they consistently return a Promise. Even though they don't await anything, they use early-return and throw which, in an async function, are translated to promise resolution and rejection, respectively.

Also, JavaScript is inherently single-threaded (well, unless you are using some kind of Workers or Worklets, but I presume you would have mentioned that). Yes, "async" code is delayed to the microtask queue, so there might be some odd timing issues, but:

If Safari implements async functions according to the spec, the body of the function runs synchronously all the way to the first await expression – and in this case, as you've pointed out, there is none, i.e. the whole function should actually already be executed synchronously, as you suggest.

I know it might be difficult, but I'd really appreciate a reproducible example. I have Safari at my disposal so no problem if it's just one of its quirks. Without that, I'm afraid I won't be able to help you :(

jiripudil commented 1 year ago

Hi, I'm closing this for inactivity. Feel free to reopen this issue if you have further insights into the problem.