Consider removing some assertions to improve the coding style of promise chaining for MozPromise

RESOLVED FIXED in Firefox 48

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Consider the following code:

TEST(MozPromise, Chaining)
{
  AutoTaskQueue atq;
  RefPtr<TaskQueue> queue = atq.Queue();
  RunOnTaskQueue(queue, [queue] () {
    auto p = TestPromise::CreateAndResolve(42, __func__);
    for (int i = 0; i < 1000; ++i) {
      p = p->Then(queue, __func__,
        [] (int aVal) {
          EXPECT_EQ(aVal, 42);
        },
        [] () {}
      )->CompletionPromise();
    }
  });
  queue->BeginShutdown();
}

The assertion at [1] requires us to hold the result of each Then() in an intermediate MozPromiseRequestHolder so we can disconnect the promise before shutting down the task queue.

Therefore we can't do
p->Then()->CompletionPromise()
 ->Then()->CompletionPromise()
 ->Then()->CompletionPromise()
 ......
 ->Then()->CompletionPromise();
when we need to handle task queue shutdown correctly.

Hi Bobby,
Do you think it is OK to remove the assertion at [1] for improve the style of promise chaining?

[1] https://hg.mozilla.org/mozilla-central/file/a9e33d8c48b5ca93ca1937eba4220f681a0f05ec/xpcom/threads/MozPromise.h#l312
(Assignee)

Updated

2 years ago
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 1

2 years ago
Note we also need to remove the assertions at [1] since we can't guarantee all promises in the chain will be resolved or rejected as far as task queue shutdown is concerned.

Btw, the client code can still use MozPromiseHolder to ensure the promise is resolved or rejected before destruction.

[1] https://hg.mozilla.org/mozilla-central/file/a9e33d8c48b5ca93ca1937eba4220f681a0f05ec/xpcom/threads/MozPromise.h#l653
So, the underlying motivation behind all of these assertions is to avoid the situation (which happened a lot in the pre-MozPromise days) where some shutdown ordering scenario would cause is to never deliver a notification that some other part of the code was depending on. This result in lots if scattered checks for shutdown conditions all over the code, and generally made the code fragile. So I think it's definitely important to preserve them somehow.

The basic idea is that MozPromiseHolder's destructor asserts that the promise is gone, which means that it must have either been resolved or rejected. However, the resolve/reject can fail if the task queue upon which it dispatches the runnable has been shut down. To plug this hole, we assert that _either_ the runnable was deliverted (|!mThenValue|) or the caller is known not to care (|mThenValue->IsDisconnected()|).

This doesn't really work with completion promises, both because of the ergonomic issues you mentioned, _and_ the fact that we don't support disconnecting Requests that have a completion promise:

https://hg.mozilla.org/mozilla-central/file/a9e33d8c48b5ca93ca1937eba4220f681a0f05ec/xpcom/threads/MozPromise.h#l366

So one improvement over the status quo would be to assert that _either_ the thenValue has been disconnected _or_ there's a completion promise.

But I think we can go one step further. Suppose we define a method on ThenValue called AssertIsDead(). This would require that _either_ the promise has been disconnected _or_ there's a completion promise, and _also_ invoke AssertIsDead on all the ThenValues for the CompletionPromise.

Need to think about it a bit more, but now it's dinner time. Let me know what you think.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 3

2 years ago
OK. I see the importance of the assertions to keep things in order and I like the idea of AssertIsDead() which provides customized assertions for each promise/ThenValue instances. I will work on a patch based on the concept. Thanks for the inspiration!
(In reply to JW Wang [:jwwang] from comment #3)
> OK. I see the importance of the assertions to keep things in order and I
> like the idea of AssertIsDead() which provides customized assertions for
> each promise/ThenValue instances.

> I will work on a patch based on the concept.
Awesome! Also worth noting that the reason for AssertIsDead() rather than Assert(IsDead()) is that checking dead-ness would be a data race if the ThenValue _weren't_ dead, and thus is only safe to do if the caller believe it must be the case.

>Thanks for the inspiration!

No problem! Sorry again for the delay on my end. One day I will be as fast as JW. :-)
(Assignee)

Comment 5

2 years ago
(In reply to Bobby Holley (busy) from comment #4)
> Awesome! Also worth noting that the reason for AssertIsDead() rather than
> Assert(IsDead()) is that checking dead-ness would be a data race if the
> ThenValue _weren't_ dead, and thus is only safe to do if the caller believe
> it must be the case.
Is that a convention or idiom? I don't quite understand the rationale behind these different styles.
(In reply to JW Wang [:jwwang] from comment #5)
> (In reply to Bobby Holley (busy) from comment #4)
> > Awesome! Also worth noting that the reason for AssertIsDead() rather than
> > Assert(IsDead()) is that checking dead-ness would be a data race if the
> > ThenValue _weren't_ dead, and thus is only safe to do if the caller believe
> > it must be the case.
> Is that a convention or idiom? I don't quite understand the rationale behind
> these different styles.

I'm not sure I understand the question. My argument is basically as follows:

(1) Checking IsDead() is a data race in the situation where the request is not dead.
(2) Correct code should have zero data races.
(3) As a consequence of (1) and (2), we should only call IsDead() when we are sure that it will return true.
(4) The only case where we ask a question we know the answer to is during assertions.
(5) Since assertions are the only acceptable usage of IsDead(), we should avoid exposing IsDead() directly, and instead expose only AssertIsDead().
(Assignee)

Comment 7

2 years ago
Thanks for the explanation. Now I have a better idea.
(Assignee)

Comment 8

2 years ago
Created attachment 8726636 [details]
MozReview Request: Bug 1250829 - add customized assertions for completion promises to facilitate promise chaining. r=bholley

Review commit: https://reviewboard.mozilla.org/r/38127/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38127/
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
(Assignee)

Updated

2 years ago
Attachment #8726636 - Flags: review?(bobbyholley)
Comment on attachment 8726636 [details]
MozReview Request: Bug 1250829 - add customized assertions for completion promises to facilitate promise chaining. r=bholley

https://reviewboard.mozilla.org/r/38127/#review36423

::: dom/media/gtest/TestMozPromise.cpp:233
(Diff revision 1)
> +  RefPtr<TaskQueue> queue = atq.Queue();
> +  MozPromiseRequestHolder<TestPromise> holder;
> +
> +  RunOnTaskQueue(queue, [queue, &holder] () {
> +    auto p = TestPromise::CreateAndResolve(42, __func__);
> +    int loop = 100;

I'd make this const size_t kLoopIteratos = 100;

::: dom/media/gtest/TestMozPromise.cpp:249
(Diff revision 1)
> +            holder.Disconnect();
> +            queue->BeginShutdown();

Shouldn't we assert this case was never hit?

::: xpcom/threads/MozPromise.h:350
(Diff revision 1)
> +      // We can't assert IsDisconnected() when a completion promise is present
> +      // since we don't support it. See comments in Disconnect() below.
> +      if (mCompletionPromise) {
> +        mCompletionPromise->AssertIsDead();
> +      } else {
> +        MOZ_DIAGNOSTIC_ASSERT(Request::mDisconnected);

This makes it sound like the mCompletionPromise case is a weaker assertion, which I don't think is the case anymore.

I would add a long comment that more explicitly lays out the reasoning in comment 2.

::: xpcom/threads/MozPromise.h:633
(Diff revision 1)
>      } else {
>        mChainedPromises.AppendElement(chainedPromise);
>      }
>    }
>  
> +  void AssertIsDead()

Please add a comment with the explanation from comment 6 here.

::: xpcom/threads/MozPromise.h:683
(Diff revision 1)
>    virtual ~MozPromise()
>    {
>      PROMISE_LOG("MozPromise::~MozPromise [this=%p]", this);
> +    // We can't guarantee a completion promise will always be revolved or
> +    // rejected since ResolveOrRejectRunnable might not run when dispatch fails.
> +    if (!mIsCompletionPromise) {

We should AssertIsDead here, right? We can only get here if all of the promise above us in the chain have also been destroyed, which means that ResolveOrRejectRunnable should have already invoked AssertIsDead.
Attachment #8726636 - Flags: review?(bobbyholley)
(Assignee)

Comment 10

2 years ago
https://reviewboard.mozilla.org/r/38127/#review36423

> Shouldn't we assert this case was never hit?

Which case do you mean?

> This makes it sound like the mCompletionPromise case is a weaker assertion, which I don't think is the case anymore.
> 
> I would add a long comment that more explicitly lays out the reasoning in comment 2.

Can you suggest the workding/comment to be laid here?
(In reply to JW Wang [:jwwang] from comment #10)
> https://reviewboard.mozilla.org/r/38127/#review36423
> 
> > Shouldn't we assert this case was never hit?
> 
> Which case do you mean?

The reject closure for the Then() call made in the (i == loop / 2) case.
 
> > This makes it sound like the mCompletionPromise case is a weaker assertion, which I don't think is the case anymore.
> > 
> > I would add a long comment that more explicitly lays out the reasoning in comment 2.
> 
> Can you suggest the workding/comment to be laid here?

We want to assert that this ThenValues is dead - that is to say, that there are no consumers waiting for the result. In the case of a normal ThenValue, we check that it has been disconnected, which is the way that the consumer signals that it no longer wishes to hear about the result. If this ThenValue has a completion promise (which is mutually exclusive with being disconnectable), we recursively assert that every ThenValue associated with the completion promise is dead.
(Assignee)

Comment 12

2 years ago
Created attachment 8730545 [details]
MozReview Request: Bug 1250829 - add customized assertions for completion promises to facilitate promise chaining. r=bholley

Review commit: https://reviewboard.mozilla.org/r/39985/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39985/
(Assignee)

Updated

2 years ago
Attachment #8726636 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8730545 - Flags: review?(bobbyholley)
https://reviewboard.mozilla.org/r/38125/#review36523

r=me with that fixed.

::: dom/media/gtest/TestMozPromise.cpp:233
(Diff revisions 1 - 2)
>    RefPtr<TaskQueue> queue = atq.Queue();
>    MozPromiseRequestHolder<TestPromise> holder;
>  
>    RunOnTaskQueue(queue, [queue, &holder] () {
>      auto p = TestPromise::CreateAndResolve(42, __func__);
> -    int loop = 100;
> +    const size_t kLoopIteratos = 100;

Sorry, this was a typo on my part. It should be called "kIterations".
(Assignee)

Comment 14

2 years ago
Created attachment 8730553 [details]
MozReview Request: Bug 1250829 - add customized assertions for completion promises to facilitate promise chaining. r=bobbyholley.

Review commit: https://reviewboard.mozilla.org/r/39991/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39991/
(Assignee)

Updated

2 years ago
Attachment #8730545 - Attachment is obsolete: true
Attachment #8730545 - Flags: review?(bobbyholley)
(Assignee)

Updated

2 years ago
Attachment #8730553 - Flags: review?(bobbyholley)
ReviewBoard is making it hard to see the changes, but r=me if the only change was the one in comment 13.
(Assignee)

Comment 16

2 years ago
There is a "Squashed Diff" link to show diff between revisions.
(Assignee)

Comment 17

2 years ago
Comment on attachment 8730553 [details]
MozReview Request: Bug 1250829 - add customized assertions for completion promises to facilitate promise chaining. r=bobbyholley.

Per comment 15.
Attachment #8730553 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 18

2 years ago
https://treeherder.mozilla.org/logviewer.html#?job_id=18078554&repo=try

###!!! ERROR: Potential deadlock detected:
=== Cyclical dependency starts at
--- Mutex : MozPromise Mutex calling context
  [stack trace unavailable]
--- Next dependency:
--- Mutex : TaskQueue::Queue (currently acquired)
 calling context
  [stack trace unavailable]
=== Cycle completed at
--- Mutex : MozPromise Mutex calling context
  [stack trace unavailable]
Deadlock may happen for some other execution
[2176] ###!!! ASSERTION: Potential deadlock detected:
Cyclical dependency starts at
Mutex : MozPromise Mutex
Next dependency:
Mutex : TaskQueue::Queue (currently acquired)
Cycle completed at
Mutex : MozPromise Mutex
Deadlock may happen for some other execution
: 'Error', file /builds/slave/try-lx-d-000000000000000000000/build/src/xpcom/glue/BlockingResourceBase.cpp, line 307
#01: mozilla::BlockingResourceBase::CheckAcquire [xpcom/string/nsTSubstring.h:95]
#02: mozilla::OffTheBooksMutex::Lock [xpcom/glue/BlockingResourceBase.cpp:382]
#03: mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock [xpcom/glue/Mutex.h:164]
#04: mozilla::MozPromise<RefPtr<mozilla::MediaData>, mozilla::MediaDecoderReader::NotDecodedReason, true>::AssertIsDead [xpcom/threads/MozPromise.h:644]
#05: mozilla::MozPromise<RefPtr<mozilla::MediaData>, mozilla::MediaDecoderReader::NotDecodedReason, true>::~MozPromise [xpcom/threads/MozPromise.h:693]
#06: mozilla::MozPromise<RefPtr<mozilla::MediaData>, mozilla::MediaDecoderReader::NotDecodedReason, true>::Private::~Private [memory/mozalloc/mozalloc.h:210]
#07: mozilla::MozPromiseRefcountable::Release [xpcom/threads/MozPromise.h:108]
#08: mozilla::MozPromise<RefPtr<mozilla::MediaData>, mozilla::MediaDecoderReader::NotDecodedReason, true>::ThenValueBase::ResolveOrRejectRunnable::~ResolveOrRejectRunnable [xpcom/threads/MozPromise.h:318]
#09: mozilla::MozPromise<RefPtr<mozilla::MediaData>, mozilla::MediaDecoderReader::NotDecodedReason, true>::ThenValueBase::ResolveOrRejectRunnable::~ResolveOrRejectRunnable [memory/mozalloc/mozalloc.h:210]
#10: nsRunnable::Release [xpcom/glue/nsThreadUtils.cpp:35]
#11: nsCOMPtr<nsIRunnable>::~nsCOMPtr [xpcom/glue/nsCOMPtr.h:405]
#12: nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::RemoveElementsAt [xpcom/glue/nsTArray.h:2013]
#13: nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::~nsTArray_Impl [xpcom/glue/nsTArray.h:828]
#14: mozilla::UniquePtr<mozilla::AutoTaskDispatcher::PerThreadTaskGroup, mozilla::DefaultDelete<mozilla::AutoTaskDispatcher::PerThreadTaskGroup> >::reset [xpcom/glue/nsTArray.h:2088]
#15: mozilla::AutoTaskDispatcher::TaskGroupRunnable::~TaskGroupRunnable [xpcom/glue/nsThreadUtils.h:230]
#16: mozilla::AutoTaskDispatcher::TaskGroupRunnable::~TaskGroupRunnable [memory/mozalloc/mozalloc.h:210]
#17: nsRunnable::Release [xpcom/glue/nsThreadUtils.cpp:35]
#18: nsCOMPtr<nsIRunnable>::~nsCOMPtr [xpcom/glue/nsCOMPtr.h:405]
#19: mozilla::TaskQueue::DispatchLocked [xpcom/threads/TaskQueue.cpp:74]
#20: mozilla::TaskQueue::Dispatch [xpcom/threads/TaskQueue.h:47]
#21: mozilla::AutoTaskDispatcher::DispatchTaskGroup [xpcom/threads/TaskDispatcher.h:244]
#22: mozilla::AutoTaskDispatcher::~AutoTaskDispatcher [mfbt/UniquePtr.h:287]
#23: mozilla::TaskQueue::Runner::Run [mfbt/RefPtr.h:377]
#24: nsThreadPool::Run [xpcom/threads/nsThreadPool.cpp:227]
#25: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:994]
#26: NS_ProcessNextEvent [xpcom/glue/nsThreadUtils.cpp:297]
#27: mozilla::ipc::MessagePumpForNonMainThreads::Run [ipc/glue/MessagePump.cpp:333]
#28: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:234]
#29: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:520]
#30: nsThread::ThreadFunc [xpcom/threads/nsThread.cpp:398]
#31: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:219]
#32: libpthread.so.0 + 0x6d4c

Turns out we have to fix the deadlock of bug 1257063 first.
Depends on: 1257063
(Assignee)

Comment 19

2 years ago
https://hg.mozilla.org/try/file/dec4ef98fe8c/xpcom/threads/MozPromise.h#l644
I wonder if we should lock in AssertIsDead(). Since IsDead() means no more activities should happen inside the promise so it should be safe to access mThenValues and mChainedPromises without lock.
Let's try to fix that bug, but if we can't, I might be able to be persuaded that removing the lock is ok.

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0edbdee8e573
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

2 years ago
Blocks: 1250054
You need to log in before you can comment on or make changes to this bug.