[haiku-commits] Re: haiku: hrev46368 - in src: add-ons/screen_savers/flurry bin/screen_blanker kits/screensaver preferences/screensaver
- From: John Scipione <jscipione@xxxxxxxxx>
- To: "haiku-commits@xxxxxxxxxxxxx" <haiku-commits@xxxxxxxxxxxxx>
- Date: Fri, 15 Nov 2013 18:32:43 -0500
On Fri, Nov 15, 2013 at 6:04 PM, Axel Dörfler <axeld@xxxxxxxxxxxxxxxx> wrote:
> On 11/15/2013 11:32 PM, John Scipione wrote:
>>
>> Attempting to Lock the window to start the saver in the runner thread
>> would lock up the screensaver window because the window waited for the
>> previous runner thread to quit while the runner thread would wait for
>> the window to unlock. By starting the saver in the window thread
>> instead the locking issues are sidestepped because the window is
>> already locked by App Server, it need not be locked again.
>
> Just to clear this up: windows are *never* locked by the app_server. Windows
> always lock themselves in their dispatch thread.
Alright, good to know, I meant locked by whatever locks the window in
MessageReceived() which I thought was app_server, but, apparently not!
>>> Besides that, you introduced a new problem: while fWindow->Lock() is
>>> save,
>>> even if the window does no longer exist, fView->fWindow->Lock() is not.
>>
>> Oh dear, I sure hope I didn't.
>>
>> First of all, how is it safe to call fWindow->Lock() on a window that
>> no longer exists?
>
> Read the damn code :-) That's what the looper list is for; it allows to
> check the validity of a looper without accessing it. So if the looper is no
> more, it's guaranteed that locking it will just fail even if it points to
> invalid/unmapped/random data.
>
> If fView is gone, calling Window() on a stale object is not safe.
>
>> Secondly, I check to see that fView->Window() is not NULL in _Run() so
>> it should be safe, right? Perhaps I should check if fView->Window() is
>> NULL before calling fView->Window()->LockWithTimeout()?
>
> Ever heard of multi-threading?
I'm unfortunately becoming painfully aware of it...
> The view could go away any time; if the view
> is still alive, it doesn't matter if Window() returns NULL. The problem is
> when fView already points to a deleted object; calling Window() on such an
> object could crash your app.
Okay, I will pass in the window object to the runner constructor once
again and use that to lock the window before drawing. I had assumed
that using the view to get the window would be a good shortcut, but,
apparently not.
>> I just want to add that I tried all the suggestions in #4260 before
>> going down this avenue and each fell down for one reason or another.
>> Your suggestion to call QuitAsync() setting fQuitting to true before
>> destroying the runner thread didn't work because the runner object is
>> deleted directly after which calls the destructor which calls Quit()
>> which waits for the thread to stop which runs into the deadlock.
>> Basically, it just delays the deadlock a few instructions.
>
> Huh? If you had understood what I wrote, you would have read, that the
> Quit() is supposed to happen in another thread (why should the window delete
> the runner in its own thread?).
I'm willing to admit that I might not have understood your solution
100%, but, I spawned a separate deleter thread to delete the runner.
That didn't make a difference though because the bottleneck was that
ScreenSaverRunner needed exclusive access to the window's lock and
would wait indefinitely trying to get it, no matter what thread the
runner was deleted from.
> Anyway, I think your solution is just fine then -- after you fix the
> fView->Window()->Lock() issue that is.
Okay, will do.
> The advantage of the solution I suggested is that the window would not need
> to wait 5000 usecs in the worst case, but could continue immediately. But
> that doesn't sound like a big problem in this use case, anyway :-)
Maybe there's still a way to do it your way and prevent the 5000µs
delay, but, you'll have to trust me that I tried really hard to get it
to work your way and couldn't.
Other related posts:
