Opened 15 years ago
Closed 15 years ago
#10765 closed defect (bug) (fixed)
Bigger HTTP downloads with curl class run out of memory
Reported by: | hakre | Owned by: | |
---|---|---|---|
Milestone: | 2.9 | Priority: | normal |
Severity: | normal | Version: | 2.8.4 |
Component: | General | Keywords: | has-patch developer-feedback |
Focuses: | Cc: |
Description
With a memory limit of 64M downloading a 7MB zip file consumes so much memory that the 64M limit is triggered.
It's wp-includes/http.php on line 13333 in trunk. the whole reponse is not only loaded into the memory, it's splitted in parts then (that means at least doubled).
That means: 7MB in curl memory + 7MB in the return value + 7MB in the explode functions return. So at least three times the download filessize (if not even more).
The exploded $part variable isn't used any longer. Looks like some code has been forgotten there.
Attachments (1)
Change History (8)
#2
@
15 years ago
we can possibly unset() any temporary variables once they're no longer needed, Would that free up some memory on such loads?
(Remember, Once the 7MB zip is read into memory, Its then extracted into memory as well, So there goes another 16MB(or whatever it expands to))
#3
@
15 years ago
well that "temporary" variable in that case is even not needed. so instead of setting it and in the next command unset it I just did not set it in my patch.
it would make sense to use temporary files in suche cases instead of using the memory.
normally I would prefer streams for that but that does not work with wordpress php system requirements and therefore this can be only an additional feature for those who are 5.2+ or a full implementation of a tempfile based download & zip extraction module could be helpfull. isn't there any way with curl to handle that better, so getting the data in chunks rather then at once?
#4
@
15 years ago
well that "temporary" variable in that case is even not needed.
I meant more in general of the handling, I think i've seen one or 2 locations where a copy is returned in some form whilst a temporary variable is left about elsewhere.
it would make sense to use temporary files in suche cases instead of using the memory.
True, Except that most of hte HTTP methods can only return a full item, rather than blocks of it, it'd mean duplicating the code.. As for unziping, I cant remember, but i have a feeling it may only use a string.. Not entirely sure, I know i've done it a LOT more efficient in another project i'm working on in terms of memory usage, havnt gotten around to considering porting any useable changes back to WP yet though..
#5
@
15 years ago
Well, I think that was there for before the specific solution for cURL was found. It was assumed that it was similar to the other transports, in that the headers and body would be together and need to be split. This was not the case, and the code was either there in order to go back with the old way, in case the "correct" way didn't pan out.
Once it was discovered that the correct way was in fact so, the variable was forgotten. It should be removed. As for optimizing the code base for cURL, that is probably not going to be possible, unless someone clever comes along.
#6
@
15 years ago
Let's throw this out here, commit it and go on with more important tasks. The architectural changes need to be discussed elsewhere anyway.
I've checked curl as well and it can only return the full file in memory. Not very optimal and it does not leave much space to optimize anyway.
Patch removes the code that isn't used anyway and is at least a conadidate to consume as much memory as the file surplus.