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

Avoiding Ctrl+c exits to every call #805

Closed
topherbuckley opened this issue Sep 1, 2020 · 79 comments
Closed

Avoiding Ctrl+c exits to every call #805

topherbuckley opened this issue Sep 1, 2020 · 79 comments

Comments

@topherbuckley
Copy link

Expected Behavior

Running any call (e.g. clasp push) should push to the server, then exit, returning to the normal terminal prompt.

Actual Behavior

Running any call (e.g. clasp push) pushes correctly to the server, but the terminal remains unresolved, or unclosed properly. It is responsive, but will not return to the normal terminal prompt without forcefully exiting with Ctrl+c.

Steps to Reproduce the Problem

  1. clasp push
  2. Hangs until Ctrl-c input

Specifications

  • Node version (node -v): 14.9.0
  • Version (clasp -v): 2.3.0
  • OS (Mac/Linux/Windows): Linux Ubuntu 18.04.4
@imthenachoman
Copy link

There seem to be multiple issues for the same thing. I am sure there are more but I can't find all of them. Not sure which one we want to be primary so I am posting this in all of them. Any resolution or idea what is wrong?

@topherbuckley
Copy link
Author

Sorry, I hanve't touched this since I posted back in Sept 2020 with no plans to return to this as Apps Script was too buggy for my purposes.

@imthenachoman
Copy link

I hear ya. I love how much you can do with it but it has so many one off issues that prevent it from being a full fledged solution platform. I wish Google invested more time/resources on it. Either way, I just wanted to let the devs of clasp know there are numerous other folks raising this particular issue.

@PopGoesTheWza
Copy link
Collaborator

Out of curiosity, do the issue also occurs when using npm install -g forked-clasp instead of npm install -g @google/clasp?

forked-clasp is an npm package built from https://github.com/PopGoesTheWza/clasp/tree/forked which includes many changes/fixes and was heavily refactored.

@imthenachoman
Copy link

Do I need to uninstall @google/clasp first?

@PopGoesTheWza
Copy link
Collaborator

@imthenachoman yes. Uninstall google/clasp globally before installing forked-clasp

@imthenachoman
Copy link

forked-clasp still has the same issue.

It pushes the files and then sits for 30+ seconds.

% time clasp push
└─ appsscript.json
└─ code.js
Pushed 2 files.
clasp push  1.23s user 0.26s system 5% cpu 27.725 total

@PopGoesTheWza
Copy link
Collaborator

@imthenachoman but it exits normally after the "sitting" time, right?

@imthenachoman
Copy link

Yes -- but that is the same experience with normal clasp. The clasp calls do their thing and then just sit. Been trying to find out why that is and how to fix it.

@PopGoesTheWza
Copy link
Collaborator

PopGoesTheWza commented Mar 15, 2021

Without a windows system to test, it is difficult for me to find why it "hangs" for a while before exiting.Does this happens withg every clasp commands?

@imthenachoman
Copy link

It happens on my MacOS system too. It's happening on both Windows and MacOS for me.

@nilp0inter
Copy link

I am experiencing the same issue on Linux (using nixpkgs stable version: v2.2.1). It seems to affect only to subcommands that actually make calls to the server.

@PopGoesTheWza
Copy link
Collaborator

My guess is that there are some unresolved promises/async functions which keeps the application from returning immediately. During previous refactoring I did focus on such issues, but obviously there is still some loose ends.

@imthenachoman
Copy link

I have not looked through clasp code as it is out of my depth but is there any debug flag we can use to get more verbose output? What I'm trying to figure out is exactly what function/call/line the code is "hanging" on.

@PopGoesTheWza
Copy link
Collaborator

We can try adding https://www.npmjs.com/package/why-is-node-still-running or https://www.npmjs.com/package/why-is-node-running on my fork so that I can release a version with information about what is delaying the exit.

@nilp0inter
Copy link

We can try adding https://www.npmjs.com/package/why-is-node-still-running or https://www.npmjs.com/package/why-is-node-running on my fork so that I can release a version with information about what is delaying the exit.

Sounds like a plan! I volunteer to test it out.

@PopGoesTheWza
Copy link
Collaborator

@imthenachoman @nilp0inter you can try https://www.npmjs.com/package/forked-clasp/v/2.8.0

it has the option -W or --why which will log what is still running upon exit.

i.e. clasp version --why

@nilp0inter
Copy link

@PopGoesTheWza Here is the log of running clasp list --why on my machine: https://gist.github.com/nilp0inter/13f29a38fd8ec0d569081ef23e5cf4a3

The behavior is the same. The command runs and produces all the output you see in the log in about 2 seconds, then it sits still for about 70 seconds, then it exits.

I hope this helps. Please, let me know if you require further tests.

@PopGoesTheWza
Copy link
Collaborator

@nilp0inter Your output is nearly identical to mine... (macos big sur, immediate exit)

@PopGoesTheWza
Copy link
Collaborator

@nilp0inter Can you try forked-clasp 2.8.1? There is now a new option -X --force-exit which explicitly invoke process.exit()

@nilp0inter
Copy link

I executed clasp list -X, the process exited too early not producing any output.

I executed again adding --why resulting in this output: https://gist.github.com/nilp0inter/2ce26a50e2fd3c706724b9be130d6505

@imthenachoman
Copy link

imthenachoman commented Mar 18, 2021

@PopGoesTheWza Are you saying you don't see the delay on your system? I'm seeing it on every system I have -- covering Windows, WSL, Linux, and MacOS.

@PopGoesTheWza
Copy link
Collaborator

I never experienced any significant, noticeable delay. Maybe up to 2 seconds on some rare occasions. Typically I'm using LTS node (i.e. 14.x atm) from bash/zsh on latest macOS.

@PopGoesTheWza
Copy link
Collaborator

I was able to test login and list commands on a Windows 2019 server... no delai before clasp exits.

@nilp0inter
Copy link

May this be a configuration issue in our end (or Google's end) instead of a bug?

@imthenachoman
Copy link

Is there a way to make a trimmed down version of clasp push or some command to see if it does the same thing? I don’t know NodeJS or what clasp code does to try to create it.

@nilp0inter
Copy link

It appears to work now

@PopGoesTheWza
Copy link
Collaborator

@nilp0inter like? No more issue? No delayed exit? No need for -X? Not feeling grateful for fix? ;)

@nilp0inter
Copy link

Sorry, I didn't want to sound ungrateful. Thank you very much for your hard work.

It is just I didn't think that the issue was closed, I mean, I thought you'll want to perform some more tests, maybe via @imthenachoman.

Did I hurt your feelings or something?

@PopGoesTheWza
Copy link
Collaborator

Just teasing.

Since this issue has been floating for quite a while, I will not rush to close it until some time to confirm it is fine now.

I am barely using clasp now and with TravisCI being broken, testing is minimal. In the last few days, I also bumped a few dependencies but without checking if it didn't break anything.

If all goes well I'll drop -X from next release (-Wmay still be useful in the future.)

@nilp0inter
Copy link

You scared me, @PopGoesTheWza! 🤣🤣🤣

Just to be clear, I am a little bit concern about the fix. As I said, I think the solution is brittle and will break for somebody else in the future. You have all the right to ignore my opinion, but please, consider adding an environment variable to disable the check.

Again, thank you very much for all your effort ❤️ ✨✨🍰✨✨

@PopGoesTheWza
Copy link
Collaborator

Removing all kind of is-online testing would go against the philosophy of original Google designers. Thus I will not rush taking such a route.

@PopGoesTheWza PopGoesTheWza added forked-clasp fixed in forked-clasp and removed help wanted labels Mar 19, 2021
@imthenachoman
Copy link

What was decided on the DNS? I don't see why clasp should/would use its own DNS. It should respect the network's DNS settings for various security related reasons.

I tested just clasp with forked-clasp on my home machine/network. I will check on work/corporate machine/network on Monday.

@PopGoesTheWza
Copy link
Collaborator

@imthenachoman in a nutshell:

@google/clasp use the is-online package to check if online connection is available. This package tests against OpenDns (among other addresses) which may be blocked by a firewall rule (local or network.)

When blocked it can cause some unresolved promises/async operations, which translate into a clasp command taking a long time upon exit.

To address this situation, we considered some options:

  • Eliminate any test about having an online access. I vetoed against such approach because it goes against initial design choices for Google authors of @google/clasp
  • Fix/workaround is-online usage, possibly opening an issue in this package repo. A sound approach, though I have little time and energy to do it myself.
  • Switch to is-reachable package and effectively testing connection to half a dozen 'well-known' addresses used by clasp.

The last option seems like a reasonable tradeoff, easy to implement. Latest forked-clasp npm release has it.

@imthenachoman
Copy link

Why not just check it against the API endpoint used to update GAS code? Even if it can access google.com but can't access the API endpoints then it is no good. I could see a corporate FW block the API endpoint but not google.com.

@PopGoesTheWza
Copy link
Collaborator

PopGoesTheWza commented Mar 21, 2021

@imthenachoman
Copy link

I like this solution. I vote yes for what @PopGoesTheWza has done.

@imthenachoman
Copy link

I just tested this on corporate network and it works now. Brilliant! Thank you!

@nilp0inter
Copy link

@PopGoesTheWza , I've been using the patched version daily and I am getting the Error: Looks like you are offline. intermitently. Sometimes I cannot use the application for several minutes in a row which is pretty annoying.

I've been unable to identify a pattern so far. Any ideas?

@PopGoesTheWza
Copy link
Collaborator

@nilp0inter Do you think that a build which would display which tested URL is "unreachable" would help?

@nilp0inter
Copy link

@nilp0inter Do you think that a build which would display which tested URL is "unreachable" would help?

Yes, I think so.

@PopGoesTheWza
Copy link
Collaborator

@nilp0inter try forked-clasp 2.11.1

@nilp0inter
Copy link

nilp0inter commented Mar 25, 2021

@PopGoesTheWza, I am getting this results with forked-clasp 2.11.1:

nil@xan ~                                                                                                           [10:32:32]
> $ clasp list
script.google.com ✖
console.developers.google.com ✖
console.cloud.google.com ✖
drive.google.com ✖
Error: Looks like you are offline.

nil@xan ~                                                                                                           [10:33:04]
> $ clasp login --status
script.google.com ✖
console.developers.google.com ✖
console.cloud.google.com ✖
drive.google.com ✖
You are logged in as an unknown user.

Looks like now everything is detected as down? Nevertheless I am able to access all domains from the browser, and the old version of clasp works (with the DNS drop rule disabled, of course)

@PopGoesTheWza
Copy link
Collaborator

@nilp0inter I edited my copy of the code to also display successes and tested:

~/ref/clasp forked
❯ node ./build/src/index.js login --status
script.google.com ✔
console.developers.google.com ✔
console.cloud.google.com ✔
drive.google.com ✔
script.google.com ✔
console.developers.google.com ✔
console.cloud.google.com ✔
drive.google.com ✔
You are logged in as [email protected].

What can possibly cause issues on your side?

@nilp0inter
Copy link

I have an slow Internet connection. I executed it now and get no error:

> $ clasp login --status                                                                                          [±master ●●]
You are logged in as [email protected].

@PopGoesTheWza
Copy link
Collaborator

PopGoesTheWza commented Mar 25, 2021

@nilp0inter We could try with a longer timeout? https://github.com/sindresorhus/is-reachable#options

Which value would you suggest?

@nilp0inter
Copy link

I don't know... the command without the Internet check, usually finish in less than 5 seconds. Maybe 5 seconds?

@PopGoesTheWza
Copy link
Collaborator

@nilp0inter try forked-clasp version 2.11.2. I set timeout to 10000 ms.

@PopGoesTheWza
Copy link
Collaborator

@imthenachoman have you also experienced issues like the one @nilp0inter reported?

@imthenachoman
Copy link

I have not. But let me check with others at my company.

@nilp0inter
Copy link

@PopGoesTheWza , sorry I've been very busy at work and I was unable to perform any testing.

I am now testing version 2.11.2 with increased timeout. And I am getting this results:

nil@xan ~/Project/SePScripts                                                        [11:45:14]
> $ clasp status                                                                  [±master ●●]
console.cloud.google.com ✖
Error: Looks like you are offline.

nil@xan ~/Project/SePScripts                                                        [11:45:42]
> $ clasp login --status                                                          [±master ●●]
script.google.com ✖
console.developers.google.com ✖
console.cloud.google.com ✖
drive.google.com ✖
You are logged in as an unknown user.

nil@xan ~/Project/SePScripts                                                        [11:46:00]
> $ clasp status                                                                  [±master ●●]
script.google.com ✖
console.developers.google.com ✖
console.cloud.google.com ✖
drive.google.com ✖
Error: Looks like you are offline.

nil@xan ~/Project/SePScripts                                                        [11:46:19]
> $ clasp -v                                                                      [±master ●●]
2.11.2

nil@xan ~/Project/SePScripts                                                        [11:51:52]
> $ clasp login --status                                                          [±master ●●]
script.google.com ✖
You are logged in as an unknown user.

nil@xan ~/Project/SePScripts                                                        [11:52:09]
> $ clasp login --status                                                          [±master ●●]
You are logged in as [email protected].

As you can see, it is pretty erratic, it seems to work every few tries and when it fails the errors are different. I am not sure this is related to timeouts anymore. In case you are wondering I have no proxies or any other restriction in my Internet access now.

Thank you!

@PopGoesTheWza PopGoesTheWza removed the forked-clasp fixed in forked-clasp label May 12, 2021
@PopGoesTheWza
Copy link
Collaborator

Fixed in https://github.com/google/clasp/releases/tag/v2.3.1

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 a pull request may close this issue.

4 participants