Make WordPress Core

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's profile 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)

10765.patch (515 bytes) - added by hakre 15 years ago.

Download all attachments as: .zip

Change History (8)

@hakre
15 years ago

#1 @hakre
15 years ago

  • Keywords has-patch developer-feedback added; needs-patch removed

Patch removes the code that isn't used anyway and is at least a conadidate to consume as much memory as the file surplus.

  • [10692] westi 2009.03.04 08:28:39 First pass of HTTP Proxy support. See #4011 props jacobsantos.

#2 @dd32
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 @hakre
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 @dd32
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 @jacobsantos
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 @hakre
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.

#7 @ryan
15 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [12367]) Remove unused variable. Props hakre. fixes #10765

Note: See TracTickets for help on using tickets.