refactor: flow control updates #92
Conversation
callmehiphop
added
the
do not merge
src/connection-pool.js
Outdated
| connection.end(onEndCallback); | ||
| connection.pause(); | ||
| connection.end(function(err) { | ||
| connection.cancel(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| pool.pause(); | ||
| } | ||
|
|
||
| message.nack(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -86,7 +86,7 @@ | |||
| "retry_params_name": "messaging" | |||
| }, | |||
| "StreamingPull": { | |||
| "timeout_millis": 60000, | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #92 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 8 8
Lines 848 863 +15
=====================================
+ Hits 848 863 +15
Continue to review full report at Codecov.
|
callmehiphop
removed
the
do not merge
|
@stephenplusplus @alexander-fenster this should be ready for a final review, if you would care to take a look! |
src/connection-pool.js
Outdated
| @@ -200,10 +219,13 @@ ConnectionPool.prototype.createConnection = function() { | |||
| .once(CHANNEL_ERROR_EVENT, onChannelError) | |||
| .once(CHANNEL_READY_EVENT, onChannelReady); | |||
|
|
|||
| requestStream.on('status', function(status) { | |||
| setImmediate(() => onConnectionStatus(status)); | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
Depends on google-gax release including googleapis/gax-nodejs#197
Fixes #13
Fixes #50
Fixes #66
Closes #94
Summary of changes
The overall goal of this PR is to refactor flow control.
Initially we thought a memory leak might be present (#13) however after investigating we found that writing to the server is significantly slower than reading from it and we needed to update our inventory after the write completes. Currently we update our inventory right before we write to the server, assuming that operation will resolve quickly, which sometimes it does not. This caused an increase in memory usage because we were flooding our write buffer with requests faster than they could be processed.
We also discovered a GAX issue, which would report a false positive on the completion time of writing data, this made it virtually impossible to implement any kind of flow control.
By addressing both of these issues, memory usage was cut down significantly and allows users to ack/nack messages as quickly as they please without any repercussions. It will also inadvertently fix #50 since we are now able to monitor when data is finished being written to the server.
Several smaller updates were also made in an attempt to cut back on duplicate messages, however until grpc/grpc-node#123 is resolved, its hard to say for certain if this will fix that issue entirely.