Thanks for reviewing the patch, Suresh! Those are great
suggestions. I'll try to answer your questions below.
1) I'll update that comment.
2) I don't think the observer is used to clean up and release the
latch if the thread errors out, but rather to release the latch if the
container is closed/dropped while the page is latched. I'm not sure
why we need special handling of this situation (I'd assume that the
general error handling/clean-up would work for this situation too),
but I wanted to keep as much of the old behaviour as possible. I'll
see if I can improve the comments.
If a thread errors out after the page has been latched, I think it is
the the caller's responsibility to detect the error and unlatch the
page. At least, it seems like this is the pattern being used in the
Page page = getPage(...);
// do something with page
3 and 5) I agree that the wait() should be timed. I was planning to
change it in a followup patch if that sounds OK. What would be a
reasonable timeout value? Ten seconds?
With a timed wait, there would be no need for an assert/error in the
double-latching case, since we would get a timeout error in the end.
4) I'll keep the method (it is unused, but as you said it could be
useful for debugging).
6) There are some unit tests which check that the pages are correctly
latched (for instance T_RawStoreFactory, T_Recovery, T_b2i). However,
I don't think any of them test that the latching behaves correctly
when there's a conflict. I'll see if I can write some tests for
that. Would it be OK to add such tests in a separate followup patch?