-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix/http transport memory leak #2565
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
base: master
Are you sure you want to change the base?
Changes from all commits
ab88d59
f801d90
6f7417f
86cbcda
543cea9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,11 @@ module.exports = class Http extends TransportStream { | |
| * @returns {undefined} | ||
| */ | ||
| log(info, callback) { | ||
| // Handle callback immediately since HTTP requests are already async | ||
| if (callback) { | ||
| return callback(); | ||
| } | ||
|
|
||
| this._request(info, null, null, (err, res) => { | ||
| if (res && res.statusCode !== 200) { | ||
| err = new Error(`Invalid HTTP Status Code: ${res.statusCode}`); | ||
|
|
@@ -69,12 +74,6 @@ module.exports = class Http extends TransportStream { | |
| this.emit('logged', info); | ||
| } | ||
| }); | ||
|
|
||
| // Remark: (jcrugzz) Fire and forget here so requests dont cause buffering | ||
| // and block more requests from happening? | ||
| if (callback) { | ||
| setImmediate(callback); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -192,20 +191,24 @@ module.exports = class Http extends TransportStream { | |
| * @param {string} path - request path | ||
| */ | ||
| _doBatch(options, callback, auth, path) { | ||
| // Handle callback immediately since batching is async | ||
| if (callback) { | ||
| return callback(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same q here |
||
| } | ||
|
|
||
| this.batchOptions.push(options); | ||
| if (this.batchOptions.length === 1) { | ||
| // First message stored, it's time to start the timeout! | ||
| const me = this; | ||
| this.batchCallback = callback; | ||
| this.batchTimeoutID = setTimeout(function () { | ||
| // timeout is reached, send all messages to endpoint | ||
| me.batchTimeoutID = -1; | ||
| me._doBatchRequest(me.batchCallback, auth, path); | ||
| // First message stored, start the timeout | ||
| this.batchTimeoutID = setTimeout(() => { | ||
| this.batchTimeoutID = -1; | ||
| this._doBatchRequest(null, auth, path); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is |
||
| }, this.batchInterval); | ||
| } | ||
| if (this.batchOptions.length === this.batchCount) { | ||
| // max batch count is reached, send all messages to endpoint | ||
| this._doBatchRequest(this.batchCallback, auth, path); | ||
| // max batch count reached, send immediately | ||
| clearTimeout(this.batchTimeoutID); | ||
| this.batchTimeoutID = -1; | ||
| this._doBatchRequest(null, auth, path); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same q here |
||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we return here if there's a call back? We would then exit the function before anything is logged?