Skip to content

StreamReader: Don't hang on to fully read buffers#6

Merged
jcoglan merged 1 commit intofaye:masterfrom
meteor:forget-finished-buffers
May 7, 2014
Merged

StreamReader: Don't hang on to fully read buffers#6
jcoglan merged 1 commit intofaye:masterfrom
meteor:forget-finished-buffers

Conversation

@glasser
Copy link
Copy Markdown
Contributor

@glasser glasser commented May 6, 2014

We have a socket leak in our app, and we noticed that all the sockets (despite being inactive and fully parsed) were hanging on to a buffer in queue[0]. Now, on the one hand, it was kind of nice to make the leak more obvious by inflating it :) but it should probably be fixed.

jcoglan added a commit that referenced this pull request May 7, 2014
StreamReader: Don't hang on to fully read buffers
@jcoglan jcoglan merged commit 84b6f50 into faye:master May 7, 2014
@jcoglan
Copy link
Copy Markdown
Collaborator

jcoglan commented May 7, 2014

Thanks for the patch. It's made me realise this code could be written in a far less obtuse way; I'll push a fix shortly.

@jcoglan
Copy link
Copy Markdown
Collaborator

jcoglan commented May 7, 2014

This refactoring cleans up the cursor logic and still drops buffers once fully read:

ed3907d

@jcoglan
Copy link
Copy Markdown
Collaborator

jcoglan commented May 8, 2014

This is now released in 0.3.4.

glasser added a commit to meteor/meteor that referenced this pull request May 22, 2014
The websocket-driver update includes our PR to relinquish some memory
faster. faye/websocket-driver-node#6

The sockjs update mostly consists of aligning the version of
faye-websocket it uses internally with the version that we use for our
client. It also contains this Vary: Origin change:
 sockjs/sockjs-node#130
glasser added a commit to meteor/meteor that referenced this pull request Jun 2, 2014
The websocket-driver update includes our PR to relinquish some memory
faster. faye/websocket-driver-node#6

The sockjs update mostly consists of aligning the version of
faye-websocket it uses internally with the version that we use for our
client. It also contains this Vary: Origin change:
 sockjs/sockjs-node#130
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