Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix for slab buffer retention, leading to large memory consumption #370

Merged
merged 1 commit into from

4 participants

@jmatthewsr-ms

Putting this on hold as I believe that a node core fix is the most appropriate. Buffer can also retain an 8k slab on it's own, ideally node core should emit a buffer that is not backed by any slab.

joyent/node#4660

@indexzero indexzero reopened this
@indexzero
Owner

@jmatthewsr-ms I'm reopening after @3rd-Eden bought it to my attention because because it appears the fix for underlying issue in node core was not resolve. Is this correct?

@jmatthewsr-ms

Yes. My understanding is that this won't be fixed until possibly 0.12:
jmatthewsr-ms/node-slab-memory-issues#1
joyent/node#4964

@trevnorris

You're correct. It will probably take the entire v0.11 development cycle to vet the new allocator. Also note that it currently just mirrors the way Buffers pool data. The first step is just to get the thing working. This will require a substantial overhaul. Then once everything is allocating from the same source we can focus on improving the allocation algorithm.

@3rd-Eden
Collaborator

@indexzero @mmalecki

I'd advise us to accept this pull request. I've been doing a lot of WebSocket proxy tests lately because I was interested in to seeing how our proxy solution compares to other proxies such as nginx and haproxy.

I've deployed the proxy on a 512mb joyent virtual machine running the latest ubuntu and hit it using observing/thor with:

thor --workers 6 --amount 2000 --concurrent 100 --messages 100 <url>

I saw a peak memory of 280mb before this patch. After applying this patch and re-running the command it saw a maximum of 102mb which is significant decrease. So even if this is going to be fixed in later version of Node. It makes sense to pull this asap.

@3rd-Eden
Collaborator

Re-ran the test suite and everything is passing now, it was probably a failure on travis side that marked these commits as broken.

@jmatthewsr-ms

Note that this is being worked on in node core: joyent/node#4964.

The buffer copy in this fix may not be required once node core buffer is fixed. The fix here shouldn't hurt performance if left in, but worth checking once node core is updated.

@3rd-Eden
Collaborator

@jmatthewsr-ms yes, it's being worked on but it would only be made available in node 0.12, which still another stable release away and as we have no idea how long it will take before 0.12 is released, it makes sense to merge this in IMHO (as well as in all other projects).

@jmatthewsr-ms

Agreed.

@indexzero indexzero merged commit 3763dc9 into nodejitsu:master

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 5 additions and 2 deletions.
  1. +5 −2 lib/node-http-proxy/http-proxy.js
View
7 lib/node-http-proxy/http-proxy.js
@@ -390,12 +390,15 @@ HttpProxy.prototype.proxyRequest = function (req, res, buffer) {
// Performs a WebSocket proxy operation to the location specified by
// `this.target`.
//
-HttpProxy.prototype.proxyWebSocketRequest = function (req, socket, head, buffer) {
+HttpProxy.prototype.proxyWebSocketRequest = function (req, socket, upgradeHead, buffer) {
var self = this,
outgoing = new(this.target.base),
listeners = {},
errState = false,
- CRLF = '\r\n';
+ CRLF = '\r\n',
+ //copy upgradeHead to avoid retention of large slab buffers used in node core
+ head = new Buffer(upgradeHead.length);
+ upgradeHead.copy(head);
//
// WebSocket requests must have the `GET` method and
Something went wrong with that request. Please try again.