CPP solution to P1 #1
Conversation
|
Pretty good PR. The helper function to ensure ordering is a nice approach that helps reduce the amount of copying needed. Consider doing it for java also, by making a dedicated "wrapper" function to which |
Week_1/P1_cv.cpp
Outdated
|
|
||
| void orderPrints(int number) { | ||
| unique_lock<mutex> lock(myMutex); | ||
| while(number != currIndex) { |
There was a problem hiding this comment.
you can use the wait with a predicate function as an argument (lock(myMutex, [this, number]() { return number == currIndex; });) instead of this while loop, both are valid though
Week_1/P1_cv.cpp
Outdated
| } | ||
|
|
||
| void second(function<void()> printSecond) { | ||
| orderPrints(2); |
There was a problem hiding this comment.
After you return from line 27, you are no longer holding the lock, thus the further logic of your function might be interleaved with other threads'. For example, you might have the following order of execution: orderPrints(1), orderPrints(2), and then a concurrent execution of printFirst and printSecond, resulting in gibberish in stdout.
Week_1/P1.java
Outdated
| @@ -0,0 +1,35 @@ | |||
| class Foo { | |||
| private Object lock = new Object(); | |||
| volatile private int currIndex = 1; | |||
There was a problem hiding this comment.
unnecessary volatile. confusing, as it suggest your intention to access this without a lock
Week_1/P1.java
Outdated
|
|
||
| public void first(Runnable printFirst) throws InterruptedException { | ||
| synchronized(lock) { | ||
| printFirst.run(); |
There was a problem hiding this comment.
oops, calling a callback with code you have no control of inside a lock, bad things can happen. Most importantly - it makes local reasoning impossible - so no way to judge if this code correct or not without looking at the whole program
Week_1/P1_cv.cpp
Outdated
| void first(function<void()> printFirst) { | ||
| orderPrints(1); | ||
| printFirst(); | ||
| ++currIndex; |
There was a problem hiding this comment.
BUG: shared state is modified outside a lock. It's not a nitpicking but an actual bug making this code incorrect and in some environments it might behave not as you expect
Week_1/P1_mutex_only.cpp
Outdated
|
|
||
| bool orderPrints(int number) { | ||
| myMutex.lock(); | ||
| if (currIndex == number) { |
There was a problem hiding this comment.
BUG: this should be a while loop not a single-shot if. Think why you ALWAYS have to use while.
Week_1/P1_mutex_only.cpp
Outdated
| } | ||
|
|
||
| void first(function<void()> printFirst) { | ||
| while(!orderPrints(1)); |
There was a problem hiding this comment.
oh ... you have a while loop here. While this addresses previous comment, this code structure complicates the understanding and makes more difficult to show why it's correct
Week_1/P1_mutex_only.cpp
Outdated
| void first(function<void()> printFirst) { | ||
| while(!orderPrints(1)); | ||
| printFirst(); | ||
| myMutex.unlock(); |
There was a problem hiding this comment.
very hard to reason about correctness of this code -> there's no pairing lock in the same scope.
If you really need to unlock (which is almost never neeeded) in a different scope - consider C++'s unique_lock which provides you a. scope - and you can std::move it - e.g. if function establishes a lock - you can return unique_lock and then calling scope will own it. But I'm actually not encouraging to do this - try to use scope-based locks - otherwise it's very hard to reason about correctness
There was a problem hiding this comment.
Understood. I wasn't sure how I can get a lock in my wrapper function, and then keep this lock until I finish some work in some other function. However, after now that I know a way to most all my locking/unlocking logic into the wrapper function, I will use scoped locks from now on.
Week_1/P1_mutex_only.cpp
Outdated
| mutex myMutex; | ||
| int currIndex = 1; | ||
|
|
||
| bool orderPrints(int number) { |
There was a problem hiding this comment.
+1 for attempt to factor out common code - if you do it right that helps to verify correctness of the program, but here are two issues. orderPrints() isn't a very good name choice, and second - (see in other comments)
There was a problem hiding this comment.
Thank you, I thought that factoring out code with critical sections is a good idea :)
I agree about the naming. If fact, if you have a good source -- please share how to name my functions "right" without having to guess.
VipAlekseyPetrenko
force-pushed
the
week1
branch
from
85cded4 to
85fc9d1
Compare
Week_1/P1.cpp
Outdated
| void printsWrapper(function<void()> printFunction, int turn) { | ||
| unique_lock<mutex> lock(gMutex); | ||
| gCv.wait(lock, [this, turn]{ return turn == currTurn; }); | ||
| printFunction(); |
There was a problem hiding this comment.
red flag - you unvoke printFunction() [on which implementation you don't have any control] under the lock.
Since you have no control on what that function does or what it can do when it is independently changed at some point in future - you can't apply "local reasoning" principle to judge about correctness of your code - and to assert correctness you would need to look at the whole program.
| void third(function<void()> printThird) { | ||
| printsWrapper(printThird, 3); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add here a proof of correctness in a long comment. I'll follow up with feedback once others submit the code and peer-review.
No description provided.