[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-commits
Subject: Re: branches/KDE/3.5/kdelibs/kio/kio
From: Michael Brade <brade () kde ! org>
Date: 2005-12-05 16:50:41
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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic