Page MenuHomePhabricator

Cleanup of dead mapBlockSource entries that aren't needed in FinalizeNode
ClosedPublic

Authored by CCulianu on Jul 24 2017, 16:40.

Details

Summary

A tiny memory leak exists in net_processing.cpp, this patch fixes it

Test Plan

Compile and run bitcoin. No log messages are generated to verify this is working, but if you wish, you can add some to FinalizeNode() to see stale mapBlockSource entries accumulate as bitcoin runs.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 576
Build 576: arc lint + arc unit

Event Timeline

@CCulianu : any way to accelerate the leak for testing purposes?

CCulianu planned changes to this revision.EditedJul 24 2017, 20:27

Good question. I'll investigate that and if it's the case that it can be accelerated somehow (maybe using test suite using a specifically constructed test?), I will either add a test case or figure out how it's reproduced.

CCulianu requested review of this revision.Aug 4 2017, 12:49

Bump.

I can't figure out where they leak -- but I do know it's related to the crash we had before (for which you had to add a guard to check a stale mapBlockSource entry).

I do know the only place they can get cleaned up is in the BlockSignals().BlockChecked() call (which ends up being BlockChecked() in net_processing.cpp)... so some code path isn't triggering this signal under normal operation.

I'm going to bump this and request we merge it. It does no harm, costs us nothing, and if anything it saves a few bytes of leaked memory.

It's sad I can't actually characterize *why* it's happening right now.. :/

deadalnix requested changes to this revision.Aug 4 2017, 16:57

This require a test.

src/net_processing.cpp
320

braces

323

If you are going to increment it every time, you should do it in the for where it is usually done. Then you wouldn't need a else at all.

This revision now requires changes to proceed.Aug 4 2017, 16:57

OK, will come up with a test..

src/net_processing.cpp
320

FINE!

:-|

323

That's the usual pattern for erasing from a map while iterating.

You can't increment an iterator after removing it from a map. This is the only way to really do it, AFAIK.

CCulianu requested review of this revision.Aug 5 2017, 09:43
CCulianu edited edge metadata.

I don't see an obvious/easy way to test this. This is all about an internal data structure that sometimes leaks under very specific conditions.

I'm requesting you allow this change in.

Also: about the braces -- we need to talk about that. I advise you to allow flexibility in the style guide regarding braces. I think for obvious simple one-liners like this braces aren't necessary. You risk alienating potential contributors to the project with such draconian coding style rules, if you don't mind my saying so.

Anyway bump...

Ok for the test. Please update the loop to use remove_if if possible.

src/net_processing.cpp
323

I see. This is error prone. Maybe it is best to just use remove_if then.

Unfortunately no remove_if on std::map...

I'll add braces to please you, despite my better judgement. :)

src/net_processing.cpp
323

Are you referring to my code as error-prone or the C++ way of doing it as being error prone?

It's the boiler plate way in C++ to erase while iterating through a map. Most experienced C++ programmers have seen it a thousand times.

So this construct is not any more error prone than any other code running somewhere in the universe that erases while iterating from a map. It's just std::map's own quirks here.

Now you may be able to build an argument for this use case of std::map being error prone, or perhaps poorly thought out. Maybe the designers had their reasons for invalidating iterators after erasing (in fact I know they have -- it's faster and cheaper and C++ is all about being fast).

Added braces as per @deadalnix coding style, added some additional comments

This revision is now accepted and ready to land.Aug 6 2017, 14:24
This revision was automatically updated to reflect the committed changes.