Skip to content
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

Reset responseBuffer on error, close, and new connections #19

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

schleyfox
Copy link

After writing #18 (comment) , I realized that it didn't really sit right with me.

Digging in, I discovered that the reason was that the responseBuffer isn't cleared on reconnect. All future data from future connections gets appended to the "ERROR Too many open connections\r\n" string (length 33). If the buffer is exactly that string, we throw an error, otherwise (like if the contents are "ERROR Too many open connections\r\nERROR Too many open connections\r\n" or the error message and the start of a valid response) we parse it as a regular message (same behavior as before my prior PR). Unfortunately, the first 24 bytes of that error string parse as a response header:

      magic: 69
      opcode: 82
      keyLength: 21071
      extrasLength: 82
      dataType: 32
      status: 21615
      totalBodyLength: 1864396129
      opaque: 1853431919
      cas: !!binary |-
        cGVuIGNvbm4=

We then hit https://github.com/makenotion/memjs/blob/efce2ebd228bfcb4f51905dce4a020c8a08e52b3/src/memjs/utils.ts#L154C1-L156

which means that we will wait until dataBuf is 1864396129 (totalBodyLength) + 24 bytes long (1.73GiB). We still have the close handler and the timeout handlers will trigger and clear the queue, but the buffer state will never reset. Client is forever poisoned.

We really need to rework the socket handling and connection reset logic holistically, but that's more of a lift and really would be best under exhaustive testing.

verified after the fix:

created 1023
connected 1023
closed 1023 false
MemJS: Server <localhost:11211> failed after (2) retries with error - ERROR Too many open connections
error Error: ERROR Too many open connections
    at Object.parseMessage (/Users/ben/projects/memjs/lib/memjs/utils.js:127:19)
    at Server.responseHandler (/Users/ben/projects/memjs/lib/memjs/server.js:102:32)
    at Socket.<anonymous> (/Users/ben/projects/memjs/lib/memjs/server.js:156:26)
    at Socket.emit (node:events:514:28)
    at addChunk (node:internal/streams/readable:324:12)
    at readableAddChunk (node:internal/streams/readable:297:9)
    at Readable.push (node:internal/streams/readable:234:10)
    at TCP.onStreamRead (node:internal/stream_base_commons:190:23) 1023
created 1024
connected 1024
closed 1024 false
MemJS: Server <localhost:11211> failed after (2) retries with error - ERROR Too many open connections
error Error: ERROR Too many open connections
    at Object.parseMessage (/Users/ben/projects/memjs/lib/memjs/utils.js:127:19)
    at Server.responseHandler (/Users/ben/projects/memjs/lib/memjs/server.js:102:32)
    at Socket.<anonymous> (/Users/ben/projects/memjs/lib/memjs/server.js:156:26)
    at Socket.emit (node:events:514:28)
    at addChunk (node:internal/streams/readable:324:12)
    at readableAddChunk (node:internal/streams/readable:297:9)
    at Readable.push (node:internal/streams/readable:234:10)
    at TCP.onStreamRead (node:internal/stream_base_commons:190:23) 1024

@schleyfox
Copy link
Author

Verified that the client was able to recover in our app after hitting too many connections.

@schleyfox schleyfox merged commit dff5481 into master Oct 3, 2023
1 check passed
@schleyfox schleyfox deleted the benjamin--reset-buffers-on-reconnect branch October 3, 2023 21:36
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