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.

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

Fixing race condition in yesod-bin #1381

Merged
merged 10 commits into from Apr 27, 2017
Merged

Fixing race condition in yesod-bin #1381

merged 10 commits into from Apr 27, 2017

Conversation

@psibi
Copy link
Member

@psibi psibi commented Apr 24, 2017

Stack build process emits line even after successful build process
which leads to the overwriting of the appPortVar with -1. This leads
it to a compile mode again. Pressing Return Key and rebuilding it
again makes it go, but that's just a workaround I have to do every now
and then to solve the actual issue.

I'm using a MVar based locking solution for fixing the race
condition introduced.

I have been using this patch for around 3 to 4 days in our workplace and we haven't seen any surprising behavior so far.

psibi added 3 commits Apr 20, 2017
Stack build process emittles line even after successful build process
which leads to the overwriting of the appPortVar with -1. This leads
it to a compile mode again. Pressing Return Key and rebuilding it
again makes it go, but that's just a workaround I have to do every now
and then to solve the actual issue.

I'm using a `MVar` based locking solution for fixing the race
condition introduced.
Copy link
Member

@snoyberg snoyberg left a comment

I'm not sure I fully grok the change yet, but some relatively superficial comments.


makeEmptyMVar :: MVar a -> IO ()
makeEmptyMVar mvar = do
isEmpty <- isEmptyMVar mvar

This comment has been minimized.

@snoyberg

snoyberg Apr 26, 2017
Member

It's safer to use tryTakeMVar


updateAppPort :: ByteString -> MVar (BuildOutput) -> TVar Int -> IO ()
updateAppPort bs mvar appPortVar = do
isEmpty <- isEmptyMVar mvar

This comment has been minimized.

@snoyberg

snoyberg Apr 26, 2017
Member

It seems like the logic here would be easier to follow as a TVar Bool instead of MVar Build output.

psibi added 3 commits Apr 26, 2017
@psibi
Copy link
Member Author

@psibi psibi commented Apr 26, 2017

@snoyberg Thanks for the review, I have updated the PR. I have also added DEBUG flag to cabal file which IMO helps during development of yesod-bin. I will remove it if you feel it's not needed.

-- yet.
-> TVar Int -> IO ()
updateAppPort bs buildStarted appPortVar = do
hasStarted <- readTVarIO buildStarted

This comment has been minimized.

@snoyberg

snoyberg Apr 27, 2017
Member

This looks like a race condition to me, at least in principle. Why not turn the entire function into a single STM block and perform the read and modifications atomically?

@snoyberg
Copy link
Member

@snoyberg snoyberg commented Apr 27, 2017

I'm not opposed to some debugging output, but I'd rather see it happen with a --verbose flag than CPP. I'm indifferent on whether we keep the CPP in for now though.

psibi added 3 commits Apr 27, 2017
@psibi
Copy link
Member Author

@psibi psibi commented Apr 27, 2017

Thanks for the review. Indeed making it as a single STM block is better. I have updated the PR.

@@ -1,5 +1,5 @@
name: yesod
version: 1.4.5
version: 1.4.6

This comment has been minimized.

@snoyberg

snoyberg Apr 27, 2017
Member

Not sure if this was here before or not, is it supposed to be part of this PR?

This comment has been minimized.

@psibi

psibi Apr 27, 2017
Author Member

Sorry, that was unintended. Thanks for the catch, have reverted this.

@snoyberg
Copy link
Member

@snoyberg snoyberg commented Apr 27, 2017

LGTM, thanks for the update. Merge when ready 👍

@psibi
Copy link
Member Author

@psibi psibi commented Apr 27, 2017

Thanks, will merge it once the Travis CI passes.

@psibi psibi merged commit b9e57a1 into yesodweb:master Apr 27, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@psibi
Copy link
Member Author

@psibi psibi commented Apr 27, 2017

I would love to see a new release of this version in Hackage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.