-
Notifications
You must be signed in to change notification settings - Fork 597
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
No request timeout possibly means that sockets are being left open when queries fail #446
Comments
Weird, doesn't the machine automatically time out after a while? The |
Thanks for the quick response. The timeout referred to in that doc is the timeout for TCP connections; I think my problem is with established connections that are open but not sending any data. If the TCP connection is healthy, there's no limit on how long it can be kept open which makes websockets and HTTP/2 possible. I've looked at the gcloud-node code and couldn't see any places where dataset could "forget" to run the function, except that no timeout is being set for request. The quickest solution is to see the agent's maxConnections to Infinity, which I should have done anyway, but if the connections never close the event loop will never be empty which means the process cannot gracefully exit. Therefore I think setting a timeout of something like 60 seconds just to be sure the socket is not orphaned would be a good thing to do. I could be wrong about the open socket thing though. It would be cool if a nice Googler out there who knows more about how GCD's API servers and networks work could provide some insight into whether that's even possible. Oh yeah it might help to know that I'm using a non-Google cloud server in Tokyo. I'm writing on my phone now so I'll try to get some code up within the next few hours. |
Sorry for taking so long to provide code. There are two environments in which I've experienced this. 1: Getting some entities either by date or key
And I query some
And by ID
When the socket pool gets full, both of those functions start to fail. I'm sure of this because in one environment, my API endpoints log when a request comes in successfully, but cannot complete because the 2: Server discovery using GCD I have a node module called Comrade which uses GCD to store information about servers in a cluster such as their IP addresses and to tell them when they should gracefully shut down. If you wouldn't mind looking at member.js#35, you can see that I am updating a single entity and I have a queue to prevent contention. That function is run about once every 10 seconds. Since the queue has a concurrency of 1, if any of the callbacks don't get called the queue will just keep filling up forever. The first time this happened, I thought it was a bug in |
I'm really not sure how to start approaching this issue. It would be most ideal if you provide a gcloud-node specific snippet that could be tested on its own that shows the connections are being left open, otherwise I'm inclined to say this is an issue that affects your code or is somewhere else along the pipeline. I don't see why the server would leave those connections open indefinitely, even if the callback was improperly not called. |
@jgeewax / @pcostell any insight? @richardkazuomiller sorry for how long this has been outstanding. Are you able to run some tests against our latest version? A lot of how we handle making requests has changed (for one; since this issue, we have set maxSockets to infinity). Thanks, and cool project! |
/cc @eddavisson |
I don't think I can provide any insight here. It seems like the client shouldn't depend on the specifics of how long the server might choose to keep idle sockets open. |
@stephenplusplus Sorry I missed your last comment! Also thanks for saying it's a cool project. That means a lot (^_^) As you suggested, I think this issue was (kinda) resolved when the socket pool size was increased to infinity (4188eb3). Even if the sockets were left open again, nothing bad would happen until the machine ran out of ports, which would be 10s of thousands of requests. Most deployments would probably restart the process by the time that many sockets get opened. You can probably leave it like it is and no one will die because of it. However, I disagree with @eddavisson in this case because we know that there is an amount of time in which GCD should give a response. If a request takes more than 30 seconds, there's either something wrong with the network or something wrong with GCD, and the request is either never going to give a response or fail in some other way. If it was normal for a request to take several minutes to complete, a timeout may not be appropriate, but I do not think this is the case. GCD's servers may never keep the socket open for such a long time, but the Internet is a big place and there are environments in which a server can think it's connected to a certain thing, waiting for packets, but in reality some faulty router in the middle is keeping the client connection open and dropped the outgoing connection, resulting in a half-open connection (just one example). In any case, the client has no way of knowing when or if a response will arrive, so it needs some way to close the connection after some time has passed. Also I think it's important to keep in mind that packages that prevent the event loop from becoming empty - whether it's because of rogue Since GCD is kind of a black box, whether or not to enable a socket timeout is up to you Googlers (Alphabetters?). Although it is not always the case, I think it is not unreasonable to work off the assumption that clients have a reliable connection to GCD because worrying about every edge case would be incredibly time consuming. However, adding a socket timeout would just require passing the Infinity sockets is proving to be a pretty good bandaid (or I guess two birds one stone type of deal?), and since I posted this I've moved everything critical to shiny new GCE servers so I probably won't have weird network problems anyway. This is mostly an edge case and probably won't affect me ever again, so it's more of a philosophical issue than a technical one. At this point I personally don't care which way you decide to go. Feel free to close if you want. Sorry for the long post but after six months I think it's best to get all of my feelings out there and let you guys decide what to do so we can all move on. |
TL;DR version Would adding a 60 second timeout to all requests break anything? If not, let's add a 60 second time out! If yes, let's close this issue. I don't need to hear any of the reasons either way; I feel like with the number of people involved in this now we're getting dangerously close to bikeshedding which I don't think would be doing the right thing (^_-)-☆ |
Even though it took six months, I'm glad it led to such an informative discussion! Feel free to chime in on any of our issues! I'm totally open to a sixty second timeout. PR welcome 👍 |
Awesome! I'll start working on it soon. |
@richardkazuomiller we still need you, buddy! :) No worries if you can't get around to it quickly, just a friendly ping if you're still interested. |
🤖 I have created a release \*beep\* \*boop\* --- ## [3.11.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.10.0...v3.11.0) (2021-01-09) ### Features * adds style enumeration ([#445](https://www.github.com/googleapis/nodejs-asset/issues/445)) ([28c3612](https://www.github.com/googleapis/nodejs-asset/commit/28c361225ab0cdc2d4b141b5ef02cac5f257a85b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [mocha](https://mochajs.org/) ([source](https://togithub.com/mochajs/mocha)) | devDependencies | major | [`^7.0.0` -> `^8.0.0`](https://renovatebot.com/diffs/npm/mocha/7.2.0/8.0.1) | --- ### Release Notes <details> <summary>mochajs/mocha</summary> ### [`v8.0.1`](https://togithub.com/mochajs/mocha/blob/master/CHANGELOG.md#​801--2020-06-10) [Compare Source](https://togithub.com/mochajs/mocha/compare/v8.0.0...v8.0.1) The obligatory patch after a major. #### 🐛 Fixes - [#​4328](https://togithub.com/mochajs/mocha/issues/4328): Fix `--parallel` when combined with `--watch` ([**@​boneskull**](https://togithub.com/boneskull)) ### [`v8.0.0`](https://togithub.com/mochajs/mocha/blob/master/CHANGELOG.md#​800--2020-06-10) [Compare Source](https://togithub.com/mochajs/mocha/compare/v7.2.0...v8.0.0) In this major release, Mocha adds the ability to _run tests in parallel_. Better late than never! Please note the **breaking changes** detailed below. Let's welcome [**@​giltayar**](https://togithub.com/giltayar) and [**@​nicojs**](https://togithub.com/nicojs) to the maintenance team! #### 💥 Breaking Changes - [#​4164](https://togithub.com/mochajs/mocha/issues/4164): **Mocha v8.0.0 now requires Node.js v10.0.0 or newer.** Mocha no longer supports the Node.js v8.x line ("Carbon"), which entered End-of-Life at the end of 2019 ([**@​UlisesGascon**](https://togithub.com/UlisesGascon)) - [#​4175](https://togithub.com/mochajs/mocha/issues/4175): Having been deprecated with a warning since v7.0.0, **`mocha.opts` is no longer supported** ([**@​juergba**](https://togithub.com/juergba)) ✨ **WORKAROUND:** Replace `mocha.opts` with a [configuration file](https://mochajs.org/#configuring-mocha-nodejs). - [#​4260](https://togithub.com/mochajs/mocha/issues/4260): Remove `enableTimeout()` (`this.enableTimeout()`) from the context object ([**@​craigtaub**](https://togithub.com/craigtaub)) ✨ **WORKAROUND:** Replace usage of `this.enableTimeout(false)` in your tests with `this.timeout(0)`. - [#​4315](https://togithub.com/mochajs/mocha/issues/4315): The `spec` option no longer supports a comma-delimited list of files ([**@​juergba**](https://togithub.com/juergba)) ✨ **WORKAROUND**: Use an array instead (e.g., `"spec": "foo.js,bar.js"` becomes `"spec": ["foo.js", "bar.js"]`). - [#​4309](https://togithub.com/mochajs/mocha/issues/4309): Drop support for Node.js v13.x line, which is now End-of-Life ([**@​juergba**](https://togithub.com/juergba)) - [#​4282](https://togithub.com/mochajs/mocha/issues/4282): `--forbid-only` will throw an error even if exclusive tests are avoided via `--grep` or other means ([**@​arvidOtt**](https://togithub.com/arvidOtt)) - [#​4223](https://togithub.com/mochajs/mocha/issues/4223): The context object's `skip()` (`this.skip()`) in a "before all" (`before()`) hook will no longer execute subsequent sibling hooks, in addition to hooks in child suites ([**@​juergba**](https://togithub.com/juergba)) - [#​4178](https://togithub.com/mochajs/mocha/issues/4178): Remove previously soft-deprecated APIs ([**@​wnghdcjfe**](https://togithub.com/wnghdcjfe)): - `Mocha.prototype.ignoreLeaks()` - `Mocha.prototype.useColors()` - `Mocha.prototype.useInlineDiffs()` - `Mocha.prototype.hideDiff()` #### 🎉 Enhancements - [#​4245](https://togithub.com/mochajs/mocha/issues/4245): Add ability to run tests in parallel for Node.js (see [docs](https://mochajs.org/#parallel-tests)) ([**@​boneskull**](https://togithub.com/boneskull)) ❗ See also [#​4244](https://togithub.com/mochajs/mocha/issues/4244); [Root Hook Plugins (docs)](https://mochajs.org/#root-hook-plugins) -- _root hooks must be defined via Root Hook Plugins to work in parallel mode_ - [#​4304](https://togithub.com/mochajs/mocha/issues/4304): `--require` now works with ES modules ([**@​JacobLey**](https://togithub.com/JacobLey)) - [#​4299](https://togithub.com/mochajs/mocha/issues/4299): In some circumstances, Mocha can run ES modules under Node.js v10 -- _use at your own risk!_ ([**@​giltayar**](https://togithub.com/giltayar)) #### 📖 Documentation - [#​4246](https://togithub.com/mochajs/mocha/issues/4246): Add documentation for parallel mode and Root Hook plugins ([**@​boneskull**](https://togithub.com/boneskull)) #### 🐛 Fixes (All bug fixes in Mocha v8.0.0 are also breaking changes, and are listed above) </details> --- ### Renovate configuration 📅 **Schedule**: "after 9am and before 3pm" (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻️ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-video-intelligence).
#446) * build(node): add feat in node post-processor to add client library version number in snippet metadata Co-authored-by: Benjamin E. Coe <bencoe@google.com> Source-Link: googleapis/synthtool@d337b88 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:d106724ad2a96daa1b8d88de101ba50bdb30b8df62ffa0aa2b451d93b4556641
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [mocha](https://mochajs.org/) ([source](https://togithub.com/mochajs/mocha)) | devDependencies | major | [`^7.0.0` -> `^8.0.0`](https://renovatebot.com/diffs/npm/mocha/7.2.0/8.0.1) | --- ### Release Notes <details> <summary>mochajs/mocha</summary> ### [`v8.0.1`](https://togithub.com/mochajs/mocha/blob/master/CHANGELOG.md#​801--2020-06-10) [Compare Source](https://togithub.com/mochajs/mocha/compare/v8.0.0...v8.0.1) The obligatory patch after a major. #### 🐛 Fixes - [#​4328](https://togithub.com/mochajs/mocha/issues/4328): Fix `--parallel` when combined with `--watch` ([**@​boneskull**](https://togithub.com/boneskull)) ### [`v8.0.0`](https://togithub.com/mochajs/mocha/blob/master/CHANGELOG.md#​800--2020-06-10) [Compare Source](https://togithub.com/mochajs/mocha/compare/v7.2.0...v8.0.0) In this major release, Mocha adds the ability to _run tests in parallel_. Better late than never! Please note the **breaking changes** detailed below. Let's welcome [**@​giltayar**](https://togithub.com/giltayar) and [**@​nicojs**](https://togithub.com/nicojs) to the maintenance team! #### 💥 Breaking Changes - [#​4164](https://togithub.com/mochajs/mocha/issues/4164): **Mocha v8.0.0 now requires Node.js v10.0.0 or newer.** Mocha no longer supports the Node.js v8.x line ("Carbon"), which entered End-of-Life at the end of 2019 ([**@​UlisesGascon**](https://togithub.com/UlisesGascon)) - [#​4175](https://togithub.com/mochajs/mocha/issues/4175): Having been deprecated with a warning since v7.0.0, **`mocha.opts` is no longer supported** ([**@​juergba**](https://togithub.com/juergba)) ✨ **WORKAROUND:** Replace `mocha.opts` with a [configuration file](https://mochajs.org/#configuring-mocha-nodejs). - [#​4260](https://togithub.com/mochajs/mocha/issues/4260): Remove `enableTimeout()` (`this.enableTimeout()`) from the context object ([**@​craigtaub**](https://togithub.com/craigtaub)) ✨ **WORKAROUND:** Replace usage of `this.enableTimeout(false)` in your tests with `this.timeout(0)`. - [#​4315](https://togithub.com/mochajs/mocha/issues/4315): The `spec` option no longer supports a comma-delimited list of files ([**@​juergba**](https://togithub.com/juergba)) ✨ **WORKAROUND**: Use an array instead (e.g., `"spec": "foo.js,bar.js"` becomes `"spec": ["foo.js", "bar.js"]`). - [#​4309](https://togithub.com/mochajs/mocha/issues/4309): Drop support for Node.js v13.x line, which is now End-of-Life ([**@​juergba**](https://togithub.com/juergba)) - [#​4282](https://togithub.com/mochajs/mocha/issues/4282): `--forbid-only` will throw an error even if exclusive tests are avoided via `--grep` or other means ([**@​arvidOtt**](https://togithub.com/arvidOtt)) - [#​4223](https://togithub.com/mochajs/mocha/issues/4223): The context object's `skip()` (`this.skip()`) in a "before all" (`before()`) hook will no longer execute subsequent sibling hooks, in addition to hooks in child suites ([**@​juergba**](https://togithub.com/juergba)) - [#​4178](https://togithub.com/mochajs/mocha/issues/4178): Remove previously soft-deprecated APIs ([**@​wnghdcjfe**](https://togithub.com/wnghdcjfe)): - `Mocha.prototype.ignoreLeaks()` - `Mocha.prototype.useColors()` - `Mocha.prototype.useInlineDiffs()` - `Mocha.prototype.hideDiff()` #### 🎉 Enhancements - [#​4245](https://togithub.com/mochajs/mocha/issues/4245): Add ability to run tests in parallel for Node.js (see [docs](https://mochajs.org/#parallel-tests)) ([**@​boneskull**](https://togithub.com/boneskull)) ❗ See also [#​4244](https://togithub.com/mochajs/mocha/issues/4244); [Root Hook Plugins (docs)](https://mochajs.org/#root-hook-plugins) -- _root hooks must be defined via Root Hook Plugins to work in parallel mode_ - [#​4304](https://togithub.com/mochajs/mocha/issues/4304): `--require` now works with ES modules ([**@​JacobLey**](https://togithub.com/JacobLey)) - [#​4299](https://togithub.com/mochajs/mocha/issues/4299): In some circumstances, Mocha can run ES modules under Node.js v10 -- _use at your own risk!_ ([**@​giltayar**](https://togithub.com/giltayar)) #### 📖 Documentation - [#​4246](https://togithub.com/mochajs/mocha/issues/4246): Add documentation for parallel mode and Root Hook plugins ([**@​boneskull**](https://togithub.com/boneskull)) #### 🐛 Fixes (All bug fixes in Mocha v8.0.0 are also breaking changes, and are listed above) </details> --- ### Renovate configuration 📅 **Schedule**: "after 9am and before 3pm" (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻️ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-video-intelligence).
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [yargs](https://yargs.js.org/) ([source](https://togithub.com/yargs/yargs)) | dependencies | major | [`^15.0.0` -> `^16.0.0`](https://renovatebot.com/diffs/npm/yargs/15.4.1/16.0.1) | --- ### Release Notes <details> <summary>yargs/yargs</summary> ### [`v16.0.1`](https://togithub.com/yargs/yargs/blob/master/CHANGELOG.md#​1601-httpswwwgithubcomyargsyargscomparev1600v1601-2020-09-09) [Compare Source](https://togithub.com/yargs/yargs/compare/v16.0.0...v16.0.1) ### [`v16.0.0`](https://togithub.com/yargs/yargs/blob/master/CHANGELOG.md#​1600-httpswwwgithubcomyargsyargscomparev1542v1600-2020-09-09) [Compare Source](https://togithub.com/yargs/yargs/compare/v15.4.1...v16.0.0) ##### ⚠ BREAKING CHANGES - tweaks to ESM/Deno API surface: now exports yargs function by default; getProcessArgvWithoutBin becomes hideBin; types now exported for Deno. - find-up replaced with escalade; export map added (limits importable files in Node >= 12); yarser-parser@19.x.x (new decamelize/camelcase implementation). - **usage:** single character aliases are now shown first in help output - rebase helper is no longer provided on yargs instance. - drop support for EOL Node 8 ([#​1686](https://togithub.com/yargs/yargs/issues/1686)) ##### Features - adds strictOptions() ([#​1738](https://www.github.com/yargs/yargs/issues/1738)) ([b215fba](https://www.github.com/yargs/yargs/commit/b215fba0ed6e124e5aad6cf22c8d5875661c63a3)) - **helpers:** rebase, Parser, applyExtends now blessed helpers ([#​1733](https://www.github.com/yargs/yargs/issues/1733)) ([c7debe8](https://www.github.com/yargs/yargs/commit/c7debe8eb1e5bc6ea20b5ed68026c56e5ebec9e1)) - adds support for ESM and Deno ([#​1708](https://www.github.com/yargs/yargs/issues/1708)) ([ac6d5d1](https://www.github.com/yargs/yargs/commit/ac6d5d105a75711fe703f6a39dad5181b383d6c6)) - drop support for EOL Node 8 ([#​1686](https://www.github.com/yargs/yargs/issues/1686)) ([863937f](https://www.github.com/yargs/yargs/commit/863937f23c3102f804cdea78ee3097e28c7c289f)) - i18n for ESM and Deno ([#​1735](https://www.github.com/yargs/yargs/issues/1735)) ([c71783a](https://www.github.com/yargs/yargs/commit/c71783a5a898a0c0e92ac501c939a3ec411ac0c1)) - tweaks to API surface based on user feedback ([#​1726](https://www.github.com/yargs/yargs/issues/1726)) ([4151fee](https://www.github.com/yargs/yargs/commit/4151fee4c33a97d26bc40de7e623e5b0eb87e9bb)) - **usage:** single char aliases first in help ([#​1574](https://www.github.com/yargs/yargs/issues/1574)) ([a552990](https://www.github.com/yargs/yargs/commit/a552990c120646c2d85a5c9b628e1ce92a68e797)) ##### Bug Fixes - **yargs:** add missing command(module) signature ([#​1707](https://www.github.com/yargs/yargs/issues/1707)) ([0f81024](https://www.github.com/yargs/yargs/commit/0f810245494ccf13a35b7786d021b30fc95ecad5)), closes [#​1704](https://www.github.com/yargs/yargs/issues/1704) </details> --- ### Renovate configuration :date: **Schedule**: "after 9am and before 3pm" (UTC). :vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied. :recycle: **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. :no_bell: **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-text-to-speech).
This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/452e1583-6c83-495a-ad97-fb34fc6f653f/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@3a00b7f
* feat: support for NodeAutoprovisioning ImageType PiperOrigin-RevId: 378163331 Source-Link: googleapis/googleapis@e610c3b Source-Link: googleapis/googleapis-gen@49f6968 * 🦉 Updates from OwlBot Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
🤖 I have created a release \*beep\* \*boop\* --- ## [3.11.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.10.0...v3.11.0) (2021-01-09) ### Features * adds style enumeration ([#445](https://www.github.com/googleapis/nodejs-asset/issues/445)) ([28c3612](https://www.github.com/googleapis/nodejs-asset/commit/28c361225ab0cdc2d4b141b5ef02cac5f257a85b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
I have a web app that running Node.js v0.10 and latest gcloud-node on multiple servers. I've run into two issues that occurred independent of one another but I think are symptoms of the same issue.
dataset.save
was never called, so the subsequent requests I had queued to prevent contention were never run.Before restarting the processes, I checked to see what sockets were open
In the output above, there are 15 connections to Google IPs, which remained open for several minutes before I restarted the Node processes. There are 15 because in Node 0.10 the default
maxSockets
for the global HTTP agent is5
, and there were three Node processes running on that machine.Leaving these sockets open indefinitely seems like a problem that should be fixed on the server side of GCD, but as long as I'm not missing something I propose that there should be a timeout of a few seconds for all requests sent by gcloud-node.
Anyone have any thoughts related to any of that?
Thanks in advance.
The text was updated successfully, but these errors were encountered: