threads + Completion callbacks corner case

[David Michael]

Suppose a plugin tries to make a call on a background thread that has no message loop, and provides a required completion callback. The call is not supposed to return synchronously, but I have no way of running the callback asynchronously on the correct thread.

The plugin made a mistake, but it's not clear what to do about it. I see three alternatives:

1) Run the callback on the main thread (my least favorite option).

2) Return PP_OK_COMPLETIONPENDING, but don't run the callback. Print a log message.

3) Return PP_ERROR_NO_MESSAGE_LOOP, don't run the callback.

I'm leaning towards 2 right now; it's the only behavior that doesn't break our interface contracts. But I could be swayed to do 3), since this is a real show-stopping error, and it might be more obvious this way.

Thoughts?

[Darin Fisher]

#3 seems better.  Be explicit that the function shouldn't be called that way.

-Darin

[Brett Wilson]

The PP_ERROR_NO_MESSAGE_LOOP should be returned if there's no message
loop, period. This goes for blocking calls and optional callbacks as
well. According to message_loop.h, if you want to make Pepper calls on
a thread, you need to have a message loop to "declare it" to our
system. I think this is a bit easier story for developers.

Brett

[Darin Fisher]

Even when making sync calls?

[Brett Wilson]

On Thu, Sep 6, 2012 at 12:19 PM, Darin Fisher <da...@chromium.org> wrote:
> Even when making sync calls?

I think when I wrote that I was expecting we would need some state to
"know about" your thread to enable the sync calls. That may not be
necessary, so in that case we could choose to not require this.

Brett

[Darin Fisher]

Right... I'd imagine that the calling thread would probably block on two WaitableEvents.  One to signal completion of the function call and one to signal shutdown of the system.  This is how IPC::SyncChannel basically works.

-Darin

[David Michael]

Option 3 sounds good to me. I was leaning to option 2 only because we basically guarantee right now that we will only ever return PP_OK_COMPLETIONPENDING for required callbacks, even if you do other silly things like provide an invalid PP_Resource or PP_Instance.

So far, it looks like it's trivial to only disallow calls with non-blocking callbacks on loopless threads.

That is, the following things are perfectly OK on a loopless background thread:

1) Passing a blocking completion callback to a function

2) Calling a function that doesn't require a response (reference counting funcs, pure async stuff like PostMessage)

I want to shoot for that. If it gets hard for some unforeseen reason, we can revisit requiring loops for for more stuff.

Brett

[Brett Wilson]

Okay, we should update the ppb_message_loop header then to mention
that it's only required for nonblocking callbacks.

Brett

[Darin Fisher]

On Thu, Sep 6, 2012 at 12:36 PM, David Michael <dmic...@google.com> wrote:

Option 3 sounds good to me. I was leaning to option 2 only because we basically guarantee right now that we will only ever return PP_OK_COMPLETIONPENDING for required callbacks, even if you do other silly things like provide an invalid PP_Resource or PP_Instance.

That's a really good point.  Folks using required callbacks wouldn't expect that they need to check the return value at all.  As a result, for many programs returning an error wouldn't really do much for them.  We could also crash the plugin if they make this mistake, or we could at least log something to the error console.  If we don't crash, the likely symptom will be a plugin that just appears stuck or is unable to make progress doing some task.

-Darin

[David Michael]

On Thu, Sep 6, 2012 at 1:49 PM, Darin Fisher <da...@chromium.org> wrote:

On Thu, Sep 6, 2012 at 12:36 PM, David Michael <dmic...@google.com> wrote:

Option 3 sounds good to me. I was leaning to option 2 only because we basically guarantee right now that we will only ever return PP_OK_COMPLETIONPENDING for required callbacks, even if you do other silly things like provide an invalid PP_Resource or PP_Instance.

That's a really good point.  Folks using required callbacks wouldn't expect that they need to check the return value at all.  As a result, for many programs returning an error wouldn't really do much for them.  We could also crash the plugin if they make this mistake, or we could at least log something to the error console.  If we don't crash, the likely symptom will be a plugin that just appears stuck or is unable to make progress doing some task.

Ooh, crashing sounds nice to me. This is definitely a show-stopper kind of error, and that's the most obvious way to expose it.

[Brett Wilson]

SGTM. Be sure to log before crashing :)