Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix xhr response handlers getting called 3 times. #255

Closed
wants to merge 3 commits into from
Closed

Fix xhr response handlers getting called 3 times. #255

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d140fda
Fix xhr response handlers getting called 3 times.
eskimor Nov 19, 2018
7875921
Modify IORef only when necessary
eskimor May 6, 2020
7cd37a6
Bring back `atomicModifyIORef'` for thread safety
eskimor May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion reflex-dom-core/src/Reflex/Dom/Xhr.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ import Control.Lens
import Control.Monad hiding (forM)
import Control.Monad.IO.Class
import Data.Aeson
import Data.IORef (newIORef, atomicModifyIORef')
#if MIN_VERSION_aeson(1,0,0)
import Data.Aeson.Text
#else
Expand Down Expand Up @@ -262,11 +263,16 @@ newXMLHttpRequestWithError req cb = do
iforM_ (_xhrRequestConfig_headers c) $ xmlHttpRequestSetRequestHeader xhr
maybe (return ()) (xmlHttpRequestSetResponseType xhr . fromResponseType) rt
xmlHttpRequestSetWithCredentials xhr creds
-- Avoid handler being called more than once.
-- This is needed until jsaddle/ghcjs-dom: https://github.com/ghcjs/ghcjs-dom/issues/89 is fixed.
alreadyHandled <- liftIO $ newIORef False
_ <- xmlHttpRequestOnreadystatechange xhr $ do
readyState <- xmlHttpRequestGetReadyState xhr
status <- xmlHttpRequestGetStatus xhr
statusText <- xmlHttpRequestGetStatusText xhr
when (readyState == 4) $ do
handled <- liftIO $ atomicModifyIORef' alreadyHandled $ \handled ->
if readyState == 4 then (True, handled) else (handled, handled)
when (readyState == 4 && not handled) $ do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

t <- if rt == Just XhrResponseType_Text || isNothing rt
then xmlHttpRequestGetResponseText xhr
else return Nothing
Expand Down