Skip to content

Conversation

@Demayl
Copy link

@Demayl Demayl commented Apr 6, 2017

Spotted when restoring from backup.
So memory pressure when restoring is significantly reduced.

@jcameron
Copy link
Collaborator

jcameron commented Apr 7, 2017

I'm not so sure about this patch - it would mean that navigating down the named.conf parse tree and back up it wouldn't return to the same object.

@Demayl
Copy link
Author

Demayl commented Apr 7, 2017

In worst case it must be rewritten. I tested it fast and it doesn't break anything, but something can count on this bug to work. This circular reference must be fixed, because leaked memory is huge ( around 12MB per domain restore ). In my opinion it doesn't rely on that mistake.

EDIT: Inner key 'values' in 'parent' is still a reference.

@jcameron
Copy link
Collaborator

jcameron commented Apr 8, 2017

When you say "domain restore", are you talking about Virtualmin backups and restores?

@Demayl
Copy link
Author

Demayl commented Apr 10, 2017

Yes

@jcameron
Copy link
Collaborator

Ok, what confuses me is that this patch doesn't seem related to any code that is invoked as part of the backup or restore process - if there is a memory leak, it should happen any time DNS records are listed or edited by Virtualmin.

@Demayl
Copy link
Author

Demayl commented Apr 11, 2017

Ok, you can try it too. It's a problem on long running operations ( like the restore ). Ops like list/edit are living short time.

@jcameron
Copy link
Collaborator

I wasn't able to re-produce this - how large is the named.conf file on your system?

@Demayl
Copy link
Author

Demayl commented Apr 12, 2017

1663 records, 258K
To reproduce this you must restore 2+ domains with DNS opt enabled

@jcameron
Copy link
Collaborator

So you have 1663 domains hosted by Virtualmin with DNS enabled? That's .... quite a lot.

But I'm still having trouble re-producing this issue, or seeing where the memory leak could be coming from. Even though the parsed named.conf structure has a loop, this on it's own shouldn't cause a leak unless it is serialized - which doesn't happen anywhere in the backup / restore code.

@iliaross iliaross force-pushed the master branch 2 times, most recently from 6ec1f01 to 75f0ca4 Compare April 13, 2020 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants