search

octokit / octokit.js

The all-batteries-included GitHub SDK for Browsers, Node.js, and Deno.
MIT License
6.94k stars 1.02k forks source link

Deno: `deno run` does not exit #2079

Open gr2m opened 3 years ago

gr2m commented 3 years ago

Follow up to https://github.com/octokit/octokit.js/issues/2075

What happened?

@laughedelic was able to narrow down the problem to the throttle plugin.

import { Octokit } from "https://cdn.skypack.dev/@octokit/core";
import { throttling } from "https://cdn.skypack.dev/@octokit/plugin-throttling";
const MyOctokit = Octokit.plugin(throttling);
const octokit = new MyOctokit({
  auth: Deno.env.get("GITHUB_TOKEN"),
  throttle: {
    onRateLimit: (retryAfter: any, options: any, octokit: any) => {
      return true;
    },
    onAbuseLimit: (retryAfter: any, options: any, octokit: any) => {
      octokit.log.warn(`Abuse detected for request ${options.method} ${options.url}`);
    },
  },
});

octokit.log.warn("foo");

It prints foo and hangs. If I remove everything related throttling, it prints the message and exits.

A workaround for this hanging is to add Deno.exit(0) at the end.

What did you expect to happen?

The deno process should exit, just like Node does

What the problem might be

The throttle plugin is using bottleneck by @SGrondin. Maybe it's a known issue?

I'd appreciate if someone with more deno experience could try to reproduce the hanging problem with just Bottleneck. The sourcecode where we use bottleneck is here: https://github.com/octokit/plugin-throttling.js/

wperron commented 3 years ago

Hey, Deno maintainer here :wave: I was actually just trying to use octokit to extract our release download stats and hit this issue. So far my script is as simple as it gets :

const octokit = new Octokit({ auth: Deno.env.get("GITHUB_TOKEN") });
const repo = await octokit.rest.repos.get({
  owner: "denoland",
  repo: "deno",
});

I also traced it down to plugin-throttling, I'll try to get a minimal repro with just Bottleneck next week. Don't hesitate to contact me too if you happen to be working on it.

gr2m commented 3 years ago

thank you so much @wperron for looking into it! Please let me know if there is anything that I can help with

wperron commented 3 years ago

I believe I found the reason for the hang, probably around the _startAutoCleanup function in Bottleneck (here).

In Deno, just like in the browser setInterval returns an IntervalID as opposed to the Timeout object returned by Node.js's setInterval. I have made the following little script to highlight the differences in Node.js and Deno

// Nodejs: `node this.js`
// Deno: `deno run this.js`
const base = setInterval(() => {}, 1);
if (typeof base.unref === "function") {
  base.unref();
} else {
  console.log("can't unref");
}

Without using the unref they both hang forever. unref allows Node to exit when it reaches the end of the script, regardless of the interval running in the background. I have a suspicion that the hang is also present when using octokit directly in the browser as an ES Module, but that due to the nature of how browsers work, it's simply not being noticed. I'm having issues with Skypack at the moment so Octokit simply won't load. I'll try again soon and confirm whether or not my hypothesis is correct.

fixbullshit14 commented 2 years ago

hi pls help me i have no idea

{"crashreporter_key":"43cd7e99167b919a3a11347cf6ecf9d47f733c14","bug_type":"211","timestamp":"2022-08-05 07:00:58.00 +0700","os_version":"iPhone OS 15.5 (19F77)","incident_id":"CF2E895A-09E5-4EB6-9A65-D4BF657E9960"} {"_marker":"","_preferredUserInterfaceLanguage":"id-ID","_userInterfaceLanguage":"id","_userSetRegionFormat":"ID","basebandChipset":"mav21","basebandFirmwareVersion":"","configParentUuid":"029a70d8-e2e0-48dd-8bae-c9b7a097de45","configUuid":"a84987e4-f373-42b4-a2d7-735b24924a8e","currentCountry":"","deviceCapacity":128,"homeCarrierCountry":"","homeCarrierName":"","isDualSim":false,"rolloverReason":"scheduled","servingCarrierName":"","startTimestamp":"2022-08-04T00:00:01Z","trialExperiments":"0","trialRollouts":"0","version":"2.3"} {"message":{"BogusFieldNotActuallyEverUsed":null,"Count":4,"daily_total_BogusMeasureNotActuallyEverUsed":null},"name":"StabilityHeartBeatCount5","sampling":100,"uuid":"3c8f638f-d492-465e-9e27-546a163beac9_2"} {"_marker":""}

st3fan commented 9 months ago

@wperron I'm still running into this today. As a workaround I'm ending my program with a Deno.exit(0) now but I was wondering if there is anything I could do or contribute to avoid that. Any ideas?

michielbdejong commented 2 months ago

Hey @gr2m nice to see you here! :) I'm also running into this issue; maybe the use of deno test helps us to trace it:

hello => ./test.ts:2:6 error: Leaks detected:

hello => ./test.ts:2:6

FAILED | 0 passed | 1 failed (377ms)

error: Test failed



It should probably be possible to extract a minimal snippet of code from [plugin-throttling.js](https://github.com/octokit/plugin-throttling.js/blob/main/src/wrap-request.ts) that calls [Bottleneck](https://www.npmjs.com/package/bottleneck) and shows this problem.
michielbdejong commented 2 months ago

Got it; opened an upstream issue: https://github.com/SGrondin/bottleneck/issues/225

wolfy1339 commented 2 months ago

The bottleneck package hasn't seen any new commits in the last 4 years.

At this point, it's best to find a new library that doesn't have their problem.