-
Notifications
You must be signed in to change notification settings - Fork 640
connection: use ~~process.nextTick~~ setImmediate #166
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
Conversation
lib/common/connection.js
Outdated
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
We should be using Pasted here for easy reading:
|
|
From the link:
We're just executing a callback. I'm inclined to stick to nextTick for convention reasons, as well as it's the model scenario for async-ifying sync methods according to the docs: process.nextTick. |
|
We should only use |
|
My only reasons for nextTick over setImmediate are based purely on the convention and nextTick documentation. I haven't seen a use of setImmediate to async a sync method, and would feel more comfortable if I did. I do see your points, however I don't think they affect us. It would take a very serious misuse of our library to stack a But, yolo. I'll switch it to setImmediate, 'cause why not. It doesn't seem to hurt anything, other than offer a minimal performance loss. Feels weird, but what doesn't in Node? (@rakyll - feel free to chime in if you prefer it to stick to nextTick) |
|
Yeah before either of us make a decision, I would like to hear what @rakyll |
|
With node v11.0, immediate queue will be able to tick between process ticks. I think that is going to solve the performance concern mentioned above. Otherwise, LGTM. See this test: https://github.com/joyent/node/blob/857975d5e7e0d7bf38577db0478d9e5ede79922e/test/simple/test-timers-immediate-queue.js |
|
@stephenplusplus You can probably merge this whenever. |
|
Woo! Thanks for saving us, Ryan. setImmediate for the win! |
connection: use setImmediate to simulate async.
|
😄 👍 |
* build: add Kokoro configs for autorelease * build: add Kokoro configs for autorelease * chore: remove CircleCI config
samples: pull in latest typeless bot, clean up some comments Source-Link: https://togithub.com/googleapis/synthtool/commit/0a68e568b6911b60bb6fd452eba4848b176031d8 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:5b05f26103855c3a15433141389c478d1d3fe088fb5d4e3217c4793f6b3f245e
🤖 I have created a release *beep* *boop* --- ## [2.0.3](https://togithub.com/googleapis/nodejs-service-control/compare/v2.0.2...v2.0.3) (2022-11-10) ### Bug Fixes * **deps:** Use google-gax v3.5.2 ([#163](https://togithub.com/googleapis/nodejs-service-control/issues/163)) ([f883825](https://togithub.com/googleapis/nodejs-service-control/commit/f88382517737c02ea2f0f9fa4e6c624c7a67c6b8)) * Preserve default values in x-goog-request-params header ([#156](https://togithub.com/googleapis/nodejs-service-control/issues/156)) ([0548559](https://togithub.com/googleapis/nodejs-service-control/commit/0548559a942b3f9830d49f9fa54aa75f259d355d)) * Regenerated protos JS and TS definitions ([#166](https://togithub.com/googleapis/nodejs-service-control/issues/166)) ([f9348ff](https://togithub.com/googleapis/nodejs-service-control/commit/f9348ff6913fe16f8dcf30dc81c96748d445c328)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
- [ ] Regenerate this pull request now. PiperOrigin-RevId: 439356405 Source-Link: googleapis/googleapis@afa2ba1 Source-Link: googleapis/googleapis-gen@3e40c17 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiM2U0MGMxN2UxNTEwYzk1ZmFiNThmYzIxNDNjY2I2MWNjZWNhNTk4OSJ9
Committer: @miraleung PiperOrigin-RevId: 310415142 Source-Author: Google APIs <noreply@google.com> Source-Date: Thu May 7 12:34:02 2020 -0700 Source-Repo: googleapis/googleapis Source-Sha: 684dfea7decfeca7a7526ea96a8e9256694dd5d8 Source-Link: googleapis/googleapis@684dfea
This is really just two types of changes in many places:
Use...
...as opposed to anything else.
Use
process.nextTickin two places to simulate an async operation.