New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ElggFile entity storing entire site config #9081

Closed
xpoarte opened this Issue Oct 29, 2015 · 12 comments

Comments

Projects
None yet
4 participants
@xpoarte

xpoarte commented Oct 29, 2015

Hello,
I am working on new versions and I have noticed that the images uploaded from file, tidypics and other plugins, slow down the performance, I found that the 'entities' of the images in versions 11 and 12 contain a lot of data, including all $vars or $params site,

I send a comparison of the same file from the file upload plugin in elgg 1.9.7 and 1.11.4
https://www.dropbox.com/s/069ti00xw9backs/elgg9.txt?dl=0
https://www.dropbox.com/s/sg1otwbsmvsyr3j/elgg11.txt?dl=0

sorry if my explanation is confusing, I do not speak English

I just found this difference and still have no idea where it is producing, the class in both versions are the same, if I find something I will tell you

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Oct 29, 2015

Member

Confirmed on 1.12

Member

beck24 commented Oct 29, 2015

Confirmed on 1.12

@beck24 beck24 changed the title from maybe bug with ElggFile data in versions +10 to ElggFile entity storing entire site config Oct 29, 2015

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 29, 2015

Member

I just ran into this 2 hours ago! The ElggFilestore object needlessly holds a copy of $CONFIG. This gets stored in memcache :(

Member

mrclay commented Oct 29, 2015

I just ran into this 2 hours ago! The ElggFilestore object needlessly holds a copy of $CONFIG. This gets stored in memcache :(

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 29, 2015

Member

I don't see how this would slow anything down, though.

Member

mrclay commented Oct 29, 2015

I don't see how this would slow anything down, though.

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Oct 29, 2015

Contributor

Copy of $CONFIG due to ef5bca6 ?

Contributor

iionly commented Oct 29, 2015

Copy of $CONFIG due to ef5bca6 ?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 29, 2015

Member

Yes.

Member

mrclay commented Oct 29, 2015

Yes.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 29, 2015

Member

If we can wait until 2.x I have a big patch coming.

Member

mrclay commented Oct 29, 2015

If we can wait until 2.x I have a big patch coming.

@xpoarte

This comment has been minimized.

Show comment
Hide comment
@xpoarte

xpoarte Oct 30, 2015

I do not understand why is necessary to add the $CONFIG to the _construct of the file entity, but in the class ElggDiskFilestore appears $this->CONFIG = $CONFIG;
Disabling it I can not distinguish the difference in performance, but when I check I find a small delay in tenths of a second, when I load 16 images, I do not think it's important the time difference.
About my doubts, the topic can be close, thanks :D

xpoarte commented Oct 30, 2015

I do not understand why is necessary to add the $CONFIG to the _construct of the file entity, but in the class ElggDiskFilestore appears $this->CONFIG = $CONFIG;
Disabling it I can not distinguish the difference in performance, but when I check I find a small delay in tenths of a second, when I load 16 images, I do not think it's important the time difference.
About my doubts, the topic can be close, thanks :D

@mrclay

This comment has been minimized.

Show comment
Hide comment
Member

mrclay commented Oct 30, 2015

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Nov 1, 2015

Contributor

I would really think it better to fix this specific issue already on Elgg 2.0 AND 1.12 and not wait for 2.1.

The questions that come to my mind:

  • is it safe to just remove the $this->CONFIG = $CONFIG line from the ElggDiskFilestore constructor?
  • do any other entity class constructors do the same or is it only happening in the ElggDiskFilestore class?
  • how to deal with existing entities that contain the whole site config?
  • how to deal with entities of 3rd party plugins that use classed extended from the ElggFile class? All these extended classed will have the same problem with their entities containing the site config.
Contributor

iionly commented Nov 1, 2015

I would really think it better to fix this specific issue already on Elgg 2.0 AND 1.12 and not wait for 2.1.

The questions that come to my mind:

  • is it safe to just remove the $this->CONFIG = $CONFIG line from the ElggDiskFilestore constructor?
  • do any other entity class constructors do the same or is it only happening in the ElggDiskFilestore class?
  • how to deal with existing entities that contain the whole site config?
  • how to deal with entities of 3rd party plugins that use classed extended from the ElggFile class? All these extended classed will have the same problem with their entities containing the site config.
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Nov 1, 2015

Member

It should be fixed as far back as 1.10 because $CONFIG (containing DB credentials) is being serialized in memcache, which is a security no no. The better fix is to just not serialize $this->filestore by implementing a __sleep method.

In no other way is this a bug; it just wastes space in memcache and slows (un)serialization. $CONFIG is an object (copied by reference) so these take up no memory. The other services that store $CONFIG are fine as they aren't stored in entities or other serialized objects.

Member

mrclay commented Nov 1, 2015

It should be fixed as far back as 1.10 because $CONFIG (containing DB credentials) is being serialized in memcache, which is a security no no. The better fix is to just not serialize $this->filestore by implementing a __sleep method.

In no other way is this a bug; it just wastes space in memcache and slows (un)serialization. $CONFIG is an object (copied by reference) so these take up no memory. The other services that store $CONFIG are fine as they aren't stored in entities or other serialized objects.

@mrclay mrclay added the security label Nov 1, 2015

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Nov 2, 2015

Contributor

I was (wrongly) assuming that the content of $CONFIG is saved as metadata for each ElggFile entity. That's why I was worried about the impact on 3rd party plugins. But as it seems "only" a memory issue (briefly tested by commenting out the problematic line in the constructor) I think there shouldn't be any fixes necessary to be done within 3rd party plugins itself. Good thing!

Memcache issue (security related) would indeed mean fixes back to 1.10 due to the support promised on the release policy page... Troublesome thing!

Contributor

iionly commented Nov 2, 2015

I was (wrongly) assuming that the content of $CONFIG is saved as metadata for each ElggFile entity. That's why I was worried about the impact on 3rd party plugins. But as it seems "only" a memory issue (briefly tested by commenting out the problematic line in the constructor) I think there shouldn't be any fixes necessary to be done within 3rd party plugins itself. Good thing!

Memcache issue (security related) would indeed mean fixes back to 1.10 due to the support promised on the release policy page... Troublesome thing!

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Nov 5, 2015

fix(memcache): don't store a copy of $CONFIG in file objects
Since filestore objects keep a copy of $CONFIG as a property, this would
get serialized with the ElggFile. We add a __sleep method to filter out
this property when serializing.

Fixes #9081
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay
Member

mrclay commented Nov 5, 2015

PR #9109

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Nov 5, 2015

fix(memcache): don't store a copy of $CONFIG in file objects
Since filestore objects keep a copy of $CONFIG as a property, this would
get serialized with the ElggFile. We add a __sleep method to filter out
this property when serializing, and __wakeup to reinitialize it.

Fixes #9081

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Nov 5, 2015

fix(memcache): don't store a copy of $CONFIG in file objects
Since filestore objects keep a copy of $CONFIG as a property, this would
get serialized with the ElggFile. We add a __sleep method to filter out
this property when serializing, and __wakeup to reinitialize it.

Fixes #9081

@mrclay mrclay closed this Nov 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment