Fix xhr response handlers getting called 3 times. #255

eskimor · 2018-11-19T14:30:36Z

onreadystatechange gets fired four times:

    1 (OPENED): the request starts
    2 (HEADERS_RECEIVED): the HTTP headers have been received
    3 (LOADING): the response begins to download
    4 (DONE): the response has been downloaded

The problem: between 2, 3 and 4 barely any time passes. Due to the
asynchronous nature of ghcjs and even worse with jsaddle the following
seems to happen:

onreadystatechange triggers, because of a transition from 1 to 2, so
the reflex-dom handler gets called (eventually). Unfortunately, when the
handler finally gets executed, readystate might already be in state 4, so
the check in Reflex.Dom.Xhr:

when (readyState == 4) $ do
...

passes and the user callback will be called. The problem, afterwards the
handler will be called again for the transitions from 2 to 3 and from 3
to 4, resulting in the user callback getting called three times instead
of once.

At first, I simply disconnected the event handler in the handler, which
worked. But depending on the actual implementation and ghcjs/jsaddle
internals this solution might fail itself for some race condition, so I
decided to simply encode my intent: Check and set atomically whether or
not the caller has already been called or not, and don't do anything if
that was already the case.

ryantrinkle · 2018-11-19T16:22:03Z

Can't we just capture the readyState before returning from the original handler?

eskimor · 2018-11-19T19:40:28Z

You mean the JS handler that gets registered that forwards to the ghcjs runtime? That would work too, but we could not use the nice GHCJS.DOM on then :-(

eskimor · 2018-11-20T14:30:35Z

Actually there is an even easier solution, just use on load instead of on readystatechanged: https://developer.mozilla.org/en-US/docs/Web/Events/load

I will fix this PR accordingly.

eskimor · 2018-11-20T17:54:54Z

Actually, I am not sure this is a good idea: It adds the risk of new browser incompatibility issues.

luigy · 2019-01-09T23:37:08Z

This looks related to #234 ? Wondering if you were using an older version as mentioned in the other PR and if you can repro this with a newer version

eskimor · 2019-01-10T00:13:20Z

The other PR essentially downgrades ghcjs-dom, this suggests this might be a regression in ghcjs-dom. I will check that out, when I have some time.

luigy · 2019-01-10T20:45:44Z

Ah right, I got that backwards, but also I just noticed that there might have also been a mixup between ghcjs-dom vs jsaddle wrt which package had an effect #234 (comment)

ali-abrar · 2019-03-08T03:33:34Z

What's the status of this PR? The comments seem inconclusive.

`onreadystatechange` gets fired four times: ``` 1 (OPENED): the request starts 2 (HEADERS_RECEIVED): the HTTP headers have been received 3 (LOADING): the response begins to download 4 (DONE): the response has been downloaded ``` The problem: between 2, 3 and 4 barely any time passes. Due to the asynchronous nature of ghcjs and even worse with jsaddle the following seems to happen: `onreadystatechange` triggers, because of a transition from 1 to 2, so the reflex-dom handler gets called (eventually). Unfortunately, when the handler finally gets executed, `readystate` might already be in state 4, so the check in Reflex.Dom.Xhr: ```haskell when (readyState == 4) $ do ... ``` passes and the user callback will be called. The problem, afterwards the handler will be called again for the transitions from 2 to 3 and from 3 t 4, resulting in the user callback getting called three times instead of once. At first, I simply disconnected the event handler in the handler, which worked. But depending on the actual implementation and ghcjs/jsaddle internals this solution might fail itself for some race condition, so I decided to simply encode my intent: Check and set atomically whether or not the caller has already been called or not, and don't do anything if that was already the case.

eskimor · 2019-03-08T08:43:13Z

@ali-abrar This fixes the issue for reflex-dom, but it most likely is just a manifestation of a regression in ghcjs-dom/jsaddle. I created an issue now and referenced it in a comment in this fix.

I will try to find some time soon to track this issue down properly, but in the meantime this PR would provide a fix for reflex-dom based applications.

ryantrinkle · 2019-10-13T21:04:47Z

I think we need to understand why this handler is marked async at all: ghcjs/jsaddle-dom#14 Hopefully @hamishmack can fill us in!

3noch · 2020-05-04T17:46:14Z

reflex-dom-core/src/Reflex/Dom/Xhr.hs

-      when (readyState == 4) $ do
+      handled <- liftIO $ atomicModifyIORef' alreadyHandled $ \handled ->
+        if readyState == 4 then (True, handled) else (handled, handled)
+      when (readyState == 4 && not handled) $ do


Seems like it would make more sense to move the handled logic into when since both are checking for readyState == 4 and it would avoid the atomicModifyIORef' when it's not relevant.

Well, I just meant to make it thread safe just in case. Otherwise in a multi-threaded context, the handler could still be called twice.

Moving the whole logic into the when would be thread safe as well but possibly be faster (fewer IORef operations) and not duplicate the readyState == 4 logic.

Ha right, now I get what you mean. Yep, that would be smarter :-)

Ok fixed (untested), can't really remember why I thought in 2018 I would need that atomicModifyIORef' in the first place.

Now I know why I thought the atomicModifyIORef' is needed, because it actually is ... stupid me My younger me, actually thought this through. Will fix in a second.

Also get rid of atomicModify.

ryantrinkle · 2020-05-06T18:47:49Z

Is there some reason we would not just make the handler synchronous? That seems like it would be the right way to solve this.

eskimor · 2020-05-06T19:04:08Z

So just using onSync here instead of on .. Or evaluate why readyStateChange is defined as an async event in the first place. Either way, I guess that should work as well.

Given that the async/sync handling is pretty hidden, I guess it just did not think of this possibility. I am pretty sure I have not tried it.

3noch · 2020-05-06T19:13:09Z

@eskimor Thanks!

We should try @ryantrinkle's suggestion. Do you happen to remember how to reproduce this?

eskimor · 2020-05-06T19:29:43Z

Reproduction is simple: Do an xhr request and log some output in the response handler, you will notice that you get the logged output three times for each request.

Back then, the issue became apparent in pact-web (chainweaver these days). I fixed it in this commit. But it is definitely simpler to reproduce this with a fresh obelisk template ... Also it's been almost two years, the issue might have been fixed in some other place already.

eskimor · 2020-05-06T19:32:38Z

If I remember correctly, it was not a Heisenbug and could be reproduced reliably.

This change prevents multiple firings of the response event. Fixes GitHub issue #467 Supersedes GitHub PR #255

tomsmalley · 2020-07-20T10:03:05Z

Superseded by #399

eskimor changed the title ~~Fix xhr response handlers getting called 3 times.~~ WIP: Fix xhr response handlers getting called 3 times. Nov 20, 2018

eskimor changed the title ~~WIP: Fix xhr response handlers getting called 3 times.~~ Fix xhr response handlers getting called 3 times. Nov 20, 2018

hSloan added bug fix Fixes an existing bug follow up Follow up with Merge Requester to take some action labels Jan 23, 2019

eskimor mentioned this pull request Mar 8, 2019

xhr request race condition readyState already 4 when it should be 1,2,3 .. ghcjs/ghcjs-dom#89

Open

eskimor force-pushed the rk-fix-xhr-duplicates branch from 73c872a to d140fda Compare March 8, 2019 08:39

alexfmpe mentioned this pull request Jun 26, 2019

performRequestAsync's response event fires three time per request obsidiansystems/obelisk#467

Closed

Ericson2314 added the non-breaking change label Dec 20, 2019

alexfmpe mentioned this pull request Apr 11, 2020

JSException in XHR request #369

Open

tomsmalley self-assigned this May 4, 2020

tomsmalley approved these changes May 4, 2020

View reviewed changes

3noch reviewed May 4, 2020

View reviewed changes

Modify IORef only when necessary

7875921

Also get rid of atomicModify.

Bring back atomicModifyIORef' for thread safety

7cd37a6

tomsmalley added a commit that referenced this pull request Jul 7, 2020

Use onSync for handling XHR onreadystatechange

1facfef

This change prevents multiple firings of the response event. Fixes GitHub issue #467 Supersedes GitHub PR #255

tomsmalley mentioned this pull request Jul 7, 2020

Use onSync for handling XHR onreadystatechange #399

Merged

tomsmalley closed this Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix xhr response handlers getting called 3 times. #255

Fix xhr response handlers getting called 3 times. #255

eskimor commented Nov 19, 2018 •

edited

Loading

ryantrinkle commented Nov 19, 2018

eskimor commented Nov 19, 2018

eskimor commented Nov 20, 2018

eskimor commented Nov 20, 2018

luigy commented Jan 9, 2019

eskimor commented Jan 10, 2019

luigy commented Jan 10, 2019

ali-abrar commented Mar 8, 2019

eskimor commented Mar 8, 2019 •

edited

Loading

ryantrinkle commented Oct 13, 2019

3noch May 4, 2020

eskimor May 5, 2020

3noch May 6, 2020

eskimor May 6, 2020

eskimor May 6, 2020

eskimor May 6, 2020

ryantrinkle commented May 6, 2020

eskimor commented May 6, 2020

3noch commented May 6, 2020 •

edited

Loading

eskimor commented May 6, 2020

eskimor commented May 6, 2020

tomsmalley commented Jul 20, 2020

Fix xhr response handlers getting called 3 times. #255

Fix xhr response handlers getting called 3 times. #255

Conversation

eskimor commented Nov 19, 2018 • edited Loading

ryantrinkle commented Nov 19, 2018

eskimor commented Nov 19, 2018

eskimor commented Nov 20, 2018

eskimor commented Nov 20, 2018

luigy commented Jan 9, 2019

eskimor commented Jan 10, 2019

luigy commented Jan 10, 2019

ali-abrar commented Mar 8, 2019

eskimor commented Mar 8, 2019 • edited Loading

ryantrinkle commented Oct 13, 2019

3noch May 4, 2020

Choose a reason for hiding this comment

eskimor May 5, 2020

Choose a reason for hiding this comment

3noch May 6, 2020

Choose a reason for hiding this comment

eskimor May 6, 2020

Choose a reason for hiding this comment

eskimor May 6, 2020

Choose a reason for hiding this comment

eskimor May 6, 2020

Choose a reason for hiding this comment

ryantrinkle commented May 6, 2020

eskimor commented May 6, 2020

3noch commented May 6, 2020 • edited Loading

eskimor commented May 6, 2020

eskimor commented May 6, 2020

tomsmalley commented Jul 20, 2020

eskimor commented Nov 19, 2018 •

edited

Loading

eskimor commented Mar 8, 2019 •

edited

Loading

3noch commented May 6, 2020 •

edited

Loading