You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/07/17 01:35:18 UTC
Review Request 23592: Fixed a deadlock bug in Mutex.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/include/process/mutex.hpp 35589d9
Diff: https://reviews.apache.org/r/23592/diff/
Testing
-------
3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
Thanks,
Jie Yu
Re: Review Request 23592: Fixed a deadlock bug in Mutex.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 17, 2014, 12:55 a.m., Benjamin Hindman wrote:
> > To Vinod's point, I think adding a comment where you declare the Owned<Promise<Nothing>> (at the top of the function) that explains that you need to grab this but set it outside of the critical section because setting it might trigger callbacks that try to reacquire the lock would be great!
>
> Ben Mahler wrote:
> Good point, let's also add the same comment to queue.hpp since it already does this without comment.
Yes, that would be great!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/#review47973
-----------------------------------------------------------
On July 16, 2014, 11:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23592/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 11:35 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/mutex.hpp 35589d9
>
> Diff: https://reviews.apache.org/r/23592/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 23592: Fixed a deadlock bug in Mutex.
Posted by Ben Mahler <be...@gmail.com>.
> On July 17, 2014, 12:55 a.m., Benjamin Hindman wrote:
> > To Vinod's point, I think adding a comment where you declare the Owned<Promise<Nothing>> (at the top of the function) that explains that you need to grab this but set it outside of the critical section because setting it might trigger callbacks that try to reacquire the lock would be great!
Good point, let's also add the same comment to queue.hpp since it already does this without comment.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/#review47973
-----------------------------------------------------------
On July 16, 2014, 11:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23592/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 11:35 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/mutex.hpp 35589d9
>
> Diff: https://reviews.apache.org/r/23592/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 23592: Fixed a deadlock bug in Mutex.
Posted by Jie Yu <yu...@gmail.com>.
> On July 17, 2014, 12:55 a.m., Benjamin Hindman wrote:
> > To Vinod's point, I think adding a comment where you declare the Owned<Promise<Nothing>> (at the top of the function) that explains that you need to grab this but set it outside of the critical section because setting it might trigger callbacks that try to reacquire the lock would be great!
>
> Ben Mahler wrote:
> Good point, let's also add the same comment to queue.hpp since it already does this without comment.
>
> Benjamin Hindman wrote:
> Yes, that would be great!
Added comment to Queue too.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/#review47973
-----------------------------------------------------------
On July 16, 2014, 11:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23592/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 11:35 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/mutex.hpp 35589d9
>
> Diff: https://reviews.apache.org/r/23592/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 23592: Fixed a deadlock bug in Mutex.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/#review47973
-----------------------------------------------------------
Ship it!
To Vinod's point, I think adding a comment where you declare the Owned<Promise<Nothing>> (at the top of the function) that explains that you need to grab this but set it outside of the critical section because setting it might trigger callbacks that try to reacquire the lock would be great!
- Benjamin Hindman
On July 16, 2014, 11:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23592/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 11:35 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/mutex.hpp 35589d9
>
> Diff: https://reviews.apache.org/r/23592/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 23592: Fixed a deadlock bug in Mutex.
Posted by Jie Yu <yu...@gmail.com>.
> On July 17, 2014, 12:25 a.m., Vinod Kone wrote:
> > 3rdparty/libprocess/include/process/mutex.hpp, lines 55-57
> > <https://reviews.apache.org/r/23592/diff/1/?file=633953#file633953line55>
> >
> > Can you add a comment on what the bug was and how you are fixing it?
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/#review47970
-----------------------------------------------------------
On July 16, 2014, 11:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23592/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 11:35 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/mutex.hpp 35589d9
>
> Diff: https://reviews.apache.org/r/23592/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 23592: Fixed a deadlock bug in Mutex.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/#review47970
-----------------------------------------------------------
3rdparty/libprocess/include/process/mutex.hpp
<https://reviews.apache.org/r/23592/#comment84216>
Can you add a comment on what the bug was and how you are fixing it?
- Vinod Kone
On July 16, 2014, 11:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23592/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 11:35 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/mutex.hpp 35589d9
>
> Diff: https://reviews.apache.org/r/23592/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 23592: Fixed a deadlock bug in Mutex.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/#review47964
-----------------------------------------------------------
Ship it!
Thanks Jie!
- Ben Mahler
On July 16, 2014, 11:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23592/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 11:35 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/mutex.hpp 35589d9
>
> Diff: https://reviews.apache.org/r/23592/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 23592: Fixed a deadlock bug in Mutex.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23592/#review47967
-----------------------------------------------------------
Patch looks great!
Reviews applied: [23592]
All tests passed.
- Mesos ReviewBot
On July 16, 2014, 11:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23592/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 11:35 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/mutex.hpp 35589d9
>
> Diff: https://reviews.apache.org/r/23592/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_filter=*Mutex* --gtest_repeat=1000
>
>
> Thanks,
>
> Jie Yu
>
>