[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: branches/KDE/3.5/kdelibs/kio/kio
From:       Michael Brade <brade () kde ! org>
Date:       2005-12-05 16:50:46
Message-ID: 200512051345.48550.brade () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


On Thursday 01 December 2005 20:12, Aaron J. Seigo wrote:
> UNBELIEVABLE.
>
> i discussed this with brade, alright. but it was NOT to revert this change.
> this change should not in practice affect anyone IMHO (since the problem
> was ONLY trigged in situations where something went wrong anyways and this
> simply protected as well as possible against it). the times when this
> affected anyone would be in situations such as kicker.
Well gee ey? What's going on with you, you're writing this almost two weeks
after I reverted, which was still *before* our discussion, we have exchanged 
about 6 emails each since then, discussing the real cause and the proper fix! 
Now pretending you didn't get my (pseudo-)patch I sent with my second last 
mail? And ranting exactly at the time I left for Venezuela, right after my 
last diploma examination, about two days before I actually wanted to start 
debugging KLibLoader, which I did tell you more than a week ago already?

I don't understand you anymore, honestly. Luckily I've got an internet 
connection now so that I can reply at all.

> and NO, kicker can't fix this because it is out of kicker's control. it's
> the confluence of various kdelibs classes each individually doing The Right
> Thing for themselves, but not working together (static object deletion
> based cleanup on app exit). in this case, particularly klibloader and
> kdirlister.
we don't actually know for sure yet.

> and NOW we hae a 3.5.0 where kicker crashes on logout EVERY TIME in the
> DEFAULT CONFIGURATION. this is beyond stupid.
Mind you, it was not THIS commit that did that, this is KDE_3_5_BRANCH and we 
still have more than enough time to fix it. Properly. Two things:
  - this is my code (sadly there are not many people yet who understand the
    big picture of KDirListerCache) and you did not tell me anything about
    your problem before committing the flawed fix, you did not even send it to
    core-devel, and you committed it during the freeze, so I don't think it's
    ok for you to complain *NOW* that Kicker crashes and that I don't want a
    really bad hack in my code that *I* need to debug later on.
  - your fix would have made it impossibly difficult to debug the crashes that
    are still left in KDirListerCache (probably about 1-2 of them) by
    violating one of KDirListerCache's core invariants. All future KDE 3.5.0
    reports would be a lot closer to "useless" with your patch in.

> i spent a fair amount of time working this through and had a long
> conversation with michael about this via private email. and now, to cover a
> theoretical issue, we have a very real practical issue.
Ok, to make this clear for the public: Aaron spend this time *before* our 
discussion and I told him why his patch was completely wrong. I reverted this 
before our main discussion as well, after telling him that the fix won't be 
in KDE 3.5.0 (suggested by Coolo), so it's not like I said something to Aaron 
and did it the other way around behind his back.

I even sent a patch that would change KDirListerCache *properly*. Here's the 
simple explanation: something (maybe KLibLoader) deletes KDirListerCache 
before deleting the KDirListers connected to it. Then when deleting the 
KDirListers they try to deregister form from KDirListerCache, failing while 
causing a new static KDirListerCache object to be created that can't do much 
but crash sooner or later. 

The obvious patch is to check in the KDirLister DTOR if the cache is still 
there, no need to spend days just to hack one of the core methods in 
KDirListerCache with the purpose to delay the crash long enough so that 
Kicker can quit before it. It took me about 2 minutes to find the problem and 
the patch---after updating, freeing space on my hd and recompiling my KDE 
(-libs) with debugging symbols, which took a lot longer.

I'll attach the 2 last mail I sent to Aaron for public reference. They 
included the proper patch.

> i'm not going to simply re-commit, because i think that kind of commit
> fighting is not useful. i'd like to request permission to recommit my
> changes so that 3.5.1 might not suck as much in such an obviously stupid
> way.
Absolutely no. I sent you the proper patch for KDirListerCache already and I 
told you why I didn't commit it yet (see attachment). I'll commit it soon. 
Soon enough for KDE 3.5.1, but not before I have debugged the problem to 
maybe find a fix for the real cause, a fix for KLibLoader, or Kicker, or 
whatever.

The least understandable part about your reaction is that you actually want to 
put your hack back in KDirListerCache right now, instead of forcing me to put 
my new patch in as soon as possible. The reasoning behind the latter I would 
at least have been able to follow. Please let's get back to sanity.

Cheers,
-- 
Michael Brade;                 KDE Developer, Student of Computer Science
  |-mail: echo brade !#|tr -d "c oh"|s\e\d 's/e/\@/2;s/$/.org/;s/bra/k/2'
  °--web: http://www.kde.org/people/michaelb.html

KDE 3: The Next Generation in Desktop Experience

["forwarded message" (message/rfc822)]



On Wednesday 23 November 2005 23:40, you wrote:
> no, i'm not. if you rely on automagic static object deletion you can't
> assume that static object A gets deleted before or after any other static
> object B.
so you want to say that KLibLoader is static then? And that the deletion order 
of KLibLoader and KDirListerCache is not defined? I'm still not sure, because 
in my code there is exactly _one_ static object only and I'm not sure which 
order you think would work (even if this order isn't guaranteed).

> > I have a static server object holding file items
> > and several non-static clients relaying the file items to the application
> > and you say the whole thing should work properly when randomly exchanging
> > the server, deleting the file items behind all KDirListers' backs?!?
>
> no, that's not at all what i'm saying. look at the bug, what caused it, and
> then consider order of deletion issues.
well...? honestly, I don't understand what you want to get at? Maybe you miss 
the possibility that right at the time of application-quit there could still 
the user be clicking on something triggering a KDirLister action, which in 
turn will access (the just deleted) KDirListerCache?

> > > > Now you're probably thinking "what the hell does this mean".
> > >
> > > actually, i know exacxtly what that means, as it's exactly what i
> > > suggested the problem was in my last email.
> >
> > hm? you never told me that the cache was deleted while the rest was still
> > alive. Anyway, that's not the point anymore.
>
> i assumed that was obvious given the patch i committed?
Then you have a serious problem with coding, eh? =;-))
To me, something like this would have been obvious and I was thinking about it 
two days ago:

KDirLister::~KDirLister()
{
  kdDebug(7003) << "-KDirLister" << endl;

+  if ( KDirListerCache::s_pSelf )
+  {
     // Stop all running jobs
     stop();
     s_pCache->forgetDirs( this );
+   }

  delete d;
}

Which actually is an optimization that I will put in regardless of the Kicker 
crash because I realized that you can create a KDirLister and then delete it 
again without doing anything else and only at the point of deletion the 
KDirListerCache is created, unnecessarily.

It will most probably fix the Kicker crash as well in your case but this is 
not the proper fix IMHO and I fear that when I put this in with the words "it 
fixes the Kicker crash" then nobody will care about KLibLoader anymore :-/

> if one of those items loaded via klibloader uses kdirlister, and if that
> item is loaded after any other kdirlister is set up (thereby setting up the
> later deletion-based disaster), then we get the crash.
I really don't understand this, sorry... if you set up a KDirLister and use 
it, then there will already be the KDLCache, so if you load another lib 
_afterwards_, why should that be the reason for the crash? It could only be 
if a newly loaded lib doesn't see the static objects that were created up 
until then, and in this case it would also crash if no KDirLister was set up 
before loading the lib.

> this is because the kdirlistercache gets cleaned up and then klibloader
> cleans up and things go bad.
So right here is the problem: *who* cleans up KDirListerCache if not 
KLibLoader? That has to be fixed.

> i don't know what you are doing for reference counting in kdirlistercache,
> but it seems that it gets messed up somewhere along the way.
nothing. Absolutely nothing. I like optimizing things and refcounting is 
unnecessary in this design :-) And it's also not the problem here because the 
Cache that is supposed to do the refcounting is deleted!

> they used to be double deleted due to klibloader. this would indeed cause
> frequent log-out crashing. i then stopped cleaning these up on application
> quit, allowing klibloader to do so and voila ... the kdirlister crash
> instead. fun times.
tell me about it... we really ought to fix this KLL thing.

> one may wish to fix klibloader (who knows what else that would break,
> though) but it would still leave kdirlister with an undocumented contract
> with the application developer.
no. The app developer has absolutely no access to KDLCache.

-- 
Michael Brade;                 KDE Developer, Student of Computer Science
  |-mail: echo brade !#|tr -d "c oh"|s\e\d 's/e/\@/2;s/$/.org/;s/bra/k/2'
  °--web: http://www.kde.org/people/michaelb.html

KDE 3: The Next Generation in Desktop Experience

[Attachment #8 (application/pgp-signature)]
["forwarded message" (message/rfc822)]



On Thursday 24 November 2005 11:36, Michael Brade wrote:
> > this is because the kdirlistercache gets cleaned up and then klibloader
> > cleans up and things go bad.
>
> So right here is the problem: *who* cleans up KDirListerCache if not
> KLibLoader? That has to be fixed.
shit, this is going up and down in my head although I should be learning... 
anyway, could it be that when loading a library, which uses KDirLister, that 
the KDLC is created statically _next to_ the klibloader, so that when exiting 
the app the order of deletion between KLL and KDLC is not defined. So it 
deletes KDLC (which *should* be deleted by KLL), then KLL, which in turn 
deletes only then the KDL? If so, we know what to fix. If not, I'm still left 
out in the rain.

I will really stop thinking about it for now and resume debugging it in 
Venezuela in December.

-- 
Michael Brade;                 KDE Developer, Student of Computer Science
  |-mail: echo brade !#|tr -d "c oh"|s\e\d 's/e/\@/2;s/$/.org/;s/bra/k/2'
  °--web: http://www.kde.org/people/michaelb.html

KDE 3: The Next Generation in Desktop Experience

[Attachment #12 (application/pgp-signature)]
[Attachment #13 (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]