[00:00] <wgrant> That is the case.
[00:00] <wgrant> So that makes complete sense.
[00:00] <wgrant> The question is whether your case is the same.
[00:01] <wgrant> It's the same cycle, but is it also caused by the object getting lost due to the reset.
[00:01] <cjwatson> Does http://paste.ubuntu.com/14059967/ help?
[00:01] <wgrant> It's a set.
[00:01] <wgrant> It certainly shouldn't.
[00:01] <cjwatson> Oh, so it is
[00:02] <wgrant> In the garbage runs there is no case in which unhook is called from within MutableValueVariable.
[00:02] <wgrant> Even if it were only added once, it would still leak.
[00:02]  * wgrant looks for any reset hooks.
[00:03] <wgrant> Ah
[00:03] <wgrant> There are none.
[00:03] <cjwatson> invalidate probably won't cause this, since it does _mark_autoreload
[00:03] <wgrant> Right, exactly.
[00:03] <wgrant> abort does invalid, which does AutoReload.
[00:03] <wgrant> Which is why this normally works.
[00:03] <wgrant> We reset between tests, but normally abort or commit first.
[00:04] <wgrant> Look at Store.reset -- it just clears the dicts.
[00:04] <cjwatson> Yeah
[00:04] <cjwatson> With a "beware" comment at the top
[00:04] <wgrant> Hm, but the object itself is presumably destroyed.
[00:04] <wgrant> Its destructor could tell its variables to GTFO, presumably.
[00:04] <wgrant> But ew __del__
[00:05] <wgrant> I wonder how slow it would be to fire hooks on them all as they are reset...
[00:05] <cjwatson> I was wondering whether the tp_traverse implementation might be incomplete
[00:06] <wgrant> Hmm.
[00:07] <wgrant> Isn't the cycle legitimate?
[00:07] <wgrant> Or does PostgresConnection not refer back to the event_system?
[00:08] <wgrant> What would the tp_traverse bug be?
[00:09] <cjwatson> Missing MutableVariableValue._event_system, possibly.  Just trying without C extensions to narrow it down
[00:09] <wgrant> Ah, possibly.
[00:10] <cjwatson> OK, still garbage from that, so probably a red herring.
[00:15] <wgrant> (Pdb) p gc.get_referents(gc.garbage[0])[0]['_event'] is gc.get_referrers(gc.garbage[0])[1]['_event']
[00:15] <wgrant> True
[00:15] <wgrant> gc.garbage[0] is the PostgresConnection
[00:15] <wgrant> So looks legit to me.
[00:15] <wgrant> Unless we can remove a reference or use a weakref, it's uncollectable.
[00:16] <wgrant> Hm, can we weakref the hook, I wonder.
[00:16] <wgrant> Presumably if the object is still known by the store, its variable will still be alive.
[00:16] <wgrant> We only need to detect changes if we're possibly going to flush them to the store, so if we're no longer in the store it's irrelevant.
[00:16] <wgrant> Right?
[00:16] <cjwatson> Yeah, and quite a few other hooks are weakreffed
[00:16] <wgrant> Oh are they
[00:17] <wgrant> So they are
[00:17] <cjwatson> Well
[00:17] <cjwatson> Their args are
[00:17] <cjwatson> Maybe not a perfect example
[00:17] <wgrant> Hmmm actually.
[00:18] <wgrant> This may not be valid.
[00:18] <wgrant> What if I have the object, query 10 million other objects so it's evicted from Storm's cache, then change the mutable value, delete my reference, then flush.
[00:19] <cjwatson> It's a little hard to see how to get away without a reset hook.
[00:20] <wgrant> Heh and even the reset hook isn't sufficient here, I don't think.
[00:20] <wgrant> Oh, unless it keeps weakrefs to even evicted objects.
[00:20] <wgrant> In which case it would.
[00:20] <wgrant> s/unless it/unless the Store/
[00:20] <cjwatson> Cache eviction could call a hook too.
[00:20] <cjwatson> But starts to sound rather slow.
[00:21] <wgrant> It'd also be a weird hook
[00:21] <wgrant> Basically a "leak pls" hook.
[00:21] <cjwatson> Or a "make changes not work" hook
[00:21] <wgrant> Because we can't unhook _detect_changes just because the object is evicted; it may be alive elsewhere.
[00:21] <cjwatson> Yeah
[00:22] <wgrant> Ah
[00:22] <wgrant> _alive is a WeakValueDictionary.
[00:22] <wgrant> _cache is separate.
[00:22] <wgrant> So a reset hook may work.
[00:23] <wgrant> It also can't be any slower than an abort.
[00:23] <cjwatson> I have to go to sleep, so will pick this up in the morning if you haven't figured it out by then ...
[00:23] <wgrant> Oh of course it needs _alive. Even if it's evicted from the cache it has to use the same object if it's reretrieved by key.
[00:23] <wgrant> I might end up wrapping our stores, will see.
[00:24] <wgrant> Or just replacing all our reset calls, short-term.
[00:24] <cjwatson> The possible slowness would be in having to go and look for hooks on ten million objects.
[00:24] <wgrant> Right, but abort and commit already do that.
[00:24] <wgrant> And we already do that for every test.
[00:24] <cjwatson> Outlawing reset would probably be a reasonable stopgap.
[00:24] <wgrant> It's terrible, but already done.
[00:25] <cjwatson> Well, outlawing it without an immediately previous abort/commit
[00:26] <cjwatson> There aren't too many of those
[00:26] <cjwatson> Anyway, sleeeeep
[00:27] <wgrant> Yup,.
[00:27] <wgrant> Night, thanks for investigating.
[00:58] <wgrant> Trying to work out why I need that Milestone construction in there.
[00:58] <wgrant> The test doesn't leave garbage unless I've created something else referring to that object since the last commit.
[01:03] <wgrant> http://pastebin.ubuntu.com/14060983/ on top of trunk is minimal so far.
[05:35] <wgrant> cjwatson: I suspect that http://paste.ubuntu.com/14071248/ will be sufficient.
[05:35] <wgrant> It turns out that object-deleted often doesn't get called at all, because it's only invoked by a weakref callback.
[05:35] <wgrant> And the objects can sometimes outlive the weakref callbacks.
[05:35] <wgrant> s/weakref callbacks/weakrefs/
[05:36] <wgrant> (obviously the callback would never be invoked anyway in the uncollectable circular case, but removing the hooks also seems to eliminate at least one case of uncollectability. so hopefully it's enough)
[11:11] <cjwatson> wgrant: I'm trying a full test run with that patch now.  Shall I land it if it checks out?
[11:11] <wgrant> cjwatson: Or even before it checks out, if you're so inclined.
[11:11] <wgrant> The Storm test suite passes with it.
[11:11] <wgrant> And it's not obviously terribly wrong.
[11:12] <cjwatson> Can you commit it to our Storm branch and I'll sort out an sdist build and an LP landing?
[11:12] <cjwatson> I agree it seems at least plausible.
[11:13] <wgrant> We may need to detach a couple of extra hooks in the deletion hook, but this fixes at least one case.
[11:13] <wgrant> If it goes well I'll also merge storm trunk tomorrow, as it fixes that restricted librarian "Processing failed" thing that we run into occasionally.
[11:16] <cjwatson> Oh?  What was the change there?
[11:16] <cjwatson> r466?
[11:19] <wgrant> If that's the thing about making "cannot send data" ish a DisconnectionError, yes.
[11:19] <cjwatson> Yeah, that
[11:19] <wgrant> Pushed up to revision 408.
[11:19] <wgrant> 3 tags updated.
[11:19] <wgrant> oh no bzr what did you do
[11:20] <cjwatson> wgrant: http://paste.ubuntu.com/14072599/ if that helps
[11:20] <wgrant> I was hoping you'd say that.
[11:21] <wgrant> 0.20<EMAIL_ADDRESS>[11:21] <cjwatson> Be glad I didn't reflexively "bzr up" first.
[11:21] <wgrant> And two lds tags
[11:21] <wgrant> Hm.
[11:21] <wgrant> I wonder if I tried a merge into it at some point and it remembered the tags.
[11:21] <wgrant> Harmless, anyway.
[11:21] <cjwatson> Presumably those are ghosts with respect to our branch, but in the same repository.
[14:41] <cjwatson> Total: 19377 tests, 0 failures, 0 errors in 195 minutes 26.711 seconds.
[14:41] <cjwatson> good sign
[16:16] <cjwatson> Yay, buildbot happy again.
[16:16] <cjwatson> wgrant: thanks!
[21:47] <wgrant> Oh good, buildbot works.
[22:02] <cjwatson> Something has to (probably).
[22:02] <wgrant> Indeed...


[08:11] <nerdsville> Hello all :)
[08:12] <nerdsville> I was interested in helping out with the Mir Desktop Env. development
[08:14] <nerdsville> How would I get started


[03:03] <sbalneav> Eveing all
[09:35] <RichEd> hi edubunteros
[09:35] <RichEd> the edubuntu meeting today is at 12h00 UTC no ?
[09:35] <RichEd> (the fridge schedule looks like it has gone beserk)
[09:35] <kgoetz> ah darn, i'm out this evening
[09:38] <RichEd> kgoetz: 12hh00 UTC is midday, not evening ...
[09:39] <kgoetz> miday UTC, so 10pm~ my time
[09:39] <kgoetz> :(
[01:44] <RichEd> [03:19] <sbalneav> Morning all
[03:28] <sbalneav> ogra: What'd I miss in the meeting this morning?
[03:54] <ogra> nothing really
[03:55] <ogra> just the summary of what was done and whats still remaining
[04:27] <bddebian> Heya
[06:28] <payan> !ltsp
[06:28] <ubotu> LTSP is the Linux Terminal Server Project, which adds thin-client support to Linux servers. See chapter 3 of the !edubuntuhandbook, http://www.ltsp.org and/or http://en.wikipedia.org/wiki/Linux_Terminal_Server_Project
[06:28] <payan> !edubuntuhandbook
[06:28] <ubotu> The Edubuntu Handbook is currently work-in-progress and can be browsed via http://doc.ubuntu.com/edubuntu/handbook/C/
[06:29] <payan> thanks ubotu
[06:37] <moquist> sbalneav: pingo?
[07:39] <jean> my clients are not able to log off? running gutsy up to date. can someone give me a hand?


[23:03] <cjwatson> wgrant: Have you had a chance to figure out how to QA https://bugs.launchpad.net/launchpad/+bug/1489674 ?
[23:03] <mup> Bug #1489674: BugNotificationBuilder.build crashes with Unauthorized on invisible private teams <403> <fallout> <qa-needstesting> <regression> <Launchpad itself:Fix Committed by wgrant> <https://launchpad.net/bugs/1489674>
[23:03] <wgrant> cjwatson: Oh yeah, I should try that.
[23:04] <wgrant> Let me see.
[23:05] <cjwatson> wgrant: Thanks
[23:05] <cjwatson> blr: Were you going to land https://code.launchpad.net/~blr/launchpad/diff-code-select-bug-1483925/+merge/268017 ?
[23:05] <blr> cjwatson: oh, forgot about that. Yes I should do
[23:06] <cjwatson> I figured that was probably buried in flu.
[23:06] <blr> thanks cjwatson
[23:36] <cjwatson> wgrant: A quick test suggests that there are various BaseMailer call sites that could trigger this too, for example modifying a branch in the web UI that has a subscription from an inaccessible private team, so I think my first job tomorrow is going to be figuring out a central fix for that.
[23:37] <cjwatson> Or as central as possible.
[23:41] <wgrant> cjwatson: Sounds reasonable. I don't know how central it can be, but it needs investigation.
[23:42] <cjwatson> Maybe BaseMailer needs to grow a rule that all uses of it need to be zopeless-style.  Fixing this "centrally" would still involve sprinkling rSP over all the various constructors.
[23:42] <wgrant> Zopeless-style?
[23:42] <cjwatson> Jobs, scripts, or whatever.  Not with a user interaction.
[23:42] <cjwatson> Whatever the correct term is.
[23:43] <wgrant> Ah, right. The defining feature is PermissiveSecurityPolicy.
[23:43] <wgrant> But that makes it rather difficult to use from the large number of email callsites that aren't presently asynchronous, unfortunately.
[23:43] <cjwatson> That's the one, thanks.
[23:44] <cjwatson> Making those async would not be too hard nowadays.
[23:44] <cjwatson> I'll have to see whether it's worth it.
[23:45] <cjwatson> I always thought sending mail directly from something like a page rendering operation was a bit odd, though.
[23:46] <wgrant> Oh, it would certainly be a positive change.
[23:46] <wgrant> The question is how much work it is to fix.
[23:47] <cjwatson> I could stick an assert getSecurityPolicy() == PermissiveSecurityPolicy into BaseMailer and see how much of the test suite explodes.
[23:48] <wgrant> All of +queue, for one.
[23:50] <cjwatson> From a quick scan I think the currently affected mailers are Branch, BranchMergeProposal, GitRepository, and PackageUpload.  All the rest are only used from jobs or scripts.
[23:51] <wgrant> oh right, answers is async nowadays, isn't it.
[23:51] <cjwatson> Answers doesn't use BaseMailer (yet).
[23:51] <wgrant> oh
[23:51] <cjwatson> But I think its mail notifications are indeed async.
[23:52] <cjwatson> Bugs notifications are mostly async, except for the send_bug_details_to_new_bug_subscribers case you ran into.
[23:53] <wgrant> If by "mostly async" you mean "lies"
[23:53] <wgrant> The footer is calculated in the webapp.
[23:53] <wgrant> Very, very expensively.
[23:53] <cjwatson> Right, but only in the new-bug case.
[23:53] <wgrant> Most bug edit timeouts go away if we make notification sending properly async.
[23:53] <cjwatson> Er, new-bug-subscriber.
[23:53] <wgrant> Nope.
[23:54] <wgrant> The webapp populates BugNotificationRecipient
[23:54] <cjwatson> Oh, yikes.
[23:54] <wgrant> So the POST request calculates the set of people who should be notified and inserts a row for each of them, including the rationale text and footer etc.
[23:55] <wgrant> The correct solution is of course a job that says "this change happened to the bug when it was in this state, now please populate bugnotification"
[23:56] <cjwatson> I'll have a look at the BaseMailer cases tomorrow at least.  Would like to spend finite time in this swamp, but draining a bit of it is a prereq for landing team-mail safely
[23:56] <cjwatson> (I strongly suspect)
[23:56] <cjwatson> Anyway, bed.    And mostly actually not, just nipping in briefly
[23:57] <wgrant> If you can make it all PermissiveSecurityPolicy then that indeed makes things substantially simpler.
[23:57] <cjwatson> Code has a suitable job framework already, so probably easy.  Soyuz might need a bit of fiddling.
[23:57] <wgrant> Yup
[23:58] <wgrant> Though there's actually already a bug about the Soyuz one.
[23:58] <wgrant> https://bugs.launchpad.net/launchpad/+bug/566339
[23:58] <mup> Bug #566339: Cannot accept package which would notify private email addresses <boobytrap> <email> <lp-soyuz> <notifications> <Launchpad itself:Triaged> <https://launchpad.net/bugs/566339>