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

Thoughts on 403/410 issue and my hacky solution. #1021

Closed
C-Lunn opened this issue Oct 7, 2021 · 12 comments
Closed

Thoughts on 403/410 issue and my hacky solution. #1021

C-Lunn opened this issue Oct 7, 2021 · 12 comments

Comments

@C-Lunn
Copy link

C-Lunn commented Oct 7, 2021

Firstly, I am by no means an expert in JS/TS so there may be a much easier solution than the one outlined here.

I've been thinking about the 410 issue a lot. For me, modifying the URL didn't work, but that's not really good enough anyway as there are other reasons videos might not be able to be played. As such, this issue needs dealing with gracefully.

ytdl() is synchronous, but the stream object initialised and returned by it relies on an async getInfo() function. As a result, it's not possible to know (as far as I'm aware) the result of this promise and deal with it gracefully from the caller. Whilst yes, if getInfo() rejects the promise the stream emits an error, there's no confirmation emitted on success, so there's no way to know that it was successful.

In the case of discord-player (or more accurately the unofficial fix for discord-player that uses ytdl directly for videos that use old codecs, link the caller queue.play() function is, in practice, synchronous (I believe the TS implementation is written asynchronously but is compiled for an ES2015 target so we get this spaghetti mess of _awaiter() functions that do not themselves support async/await), and so I see no way out of this without changing both.

However, this presents another issue -- ytdl-core has one default export (ytdl()) and other modules in the project (namely discord-ytdl-core) rely on that not changing. So, here's what I did.

  1. Create a copy of the ytdl-core folder in node-modules, and named it ytdl-core-as.
  2. Edited the package.json file so the name matches the folder ("name": "ytdl-core-as",).
  3. Opened the ytdl-core-as/lib/index.js file, and replaced the ytdl function with an asynchronous variant as follows:
/**
 * @param {string} link
 * @param {!Object} options
 * @returns {ReadableStream}
 */
 const ytdl = async (link, options) => {
  const stream = createStream(options);
  return new Promise(async (r,j) => {
    let rejected = false;
    let info = await ytdl.getInfo(link, options).catch(() => { rejected = true; });
    if(rejected) return void r(null);
    downloadFromInfoCallback(stream, info, options);
    r(stream)
  })
}

It's important everything stays as ytdl to not break everything else in the module, but we can import it under a different name to avoid conflicts.

In my solution, the promise is always accepted and if there is an issue it simply returns null (as personally I don't care what the issue was), but this is fairly trivial to also reject it with the error code if you need.

  1. Now, in discord-player's discord-player/dist/Structures/Queue.js, I can import my new module with const ytdl_as = require("ytdl-core-as");.

However, like I mentioned earlier, discord-player's play function is effectively synchronous which means we'll need to double up some code -- namely leave the old playing stuff as it was, but duplicate it inside a .then() to deal with the newly asynchronous nature of my new ytdl.

I modified the play() function like so:

if (["youtube"].includes(track.raw.source)) {
                //console.log(`line 625 for ${track.title}`);
                ytdl_as(track.url, {
                    requestOptions: {
                        headers: {
                            //header info
                        },
                    },
                    filter: "audioonly",
                    passes: 3,
                    highWaterMark: 1 << 27
                }).then((res) => {
                    //console.log(`line 636 for ${track.title}`);
                    if (!res) {
                        console.log("skipping, sending msg");
                        this.player.client.channels.cache.get(this.metadata.channel).send(`Sorry, there was a problem with \`${track.title}\`. Skipping.`);
                        const nextTrack = this.tracks.shift();
                        if(!nextTrack) {
                            this.destroy();
                            return;
                        }
                        this.play(nextTrack, { immediate: true });
                        return;
                    } else {
                        stream = res;
                        const resource = this.connection.createStream(stream, {
                            type: track.raw.source === "youtube" ? voice_1.StreamType.WebmOpus : voice_1.StreamType.Raw,
                            data: track,
                        });
                        if (options.seek) this._streamTime = options.seek;
                        this._filtersUpdate = options.filtersUpdate;
                        this.setVolume(this.options.initialVolume);
                        setTimeout(() => {
                            this.connection.playStream(resource);
                        }, __classPrivateFieldGet(this, _Queue_instances, "m", _Queue_getBufferingTimeout).call(this)).unref();
                    }
                });
                return;

This, whilst hacky, works for me.

I hope someone more skilled than I can come up with a more elegant solution.

@KAAquilla
Copy link

KAAquilla commented Oct 10, 2021

Just make it a Pull Request and you can then do install the package version from that pull request even if its not merged yet.
Like this: "npm install user/repo#pull/id/head"
Example: npm install fent/node-ytdl-core#pull/1010/head - Which would install PR #1010
Also makes it easier for others to confirm your code.

@C-Lunn
Copy link
Author

C-Lunn commented Oct 10, 2021

Just make it a Pull Request and you can then do install the package version from that pull request even if its not merged yet. Like this: "npm install user/repo#pull/id/head" Example: npm install fent/node-ytdl-core#pull/1010/head - Which would install PR #1010 Also makes it easier for others to confirm your code.

Issue with making it a PR is that it's fundamentally changing the way the module works, and I don't know if it's the best way.

@mkko120
Copy link

mkko120 commented Oct 10, 2021

@C-Lunn it even solves the 407/ECONNRESET error for me! Definetly would recommend you to do the pull request.

Tested with fork: https://github.com/mkko120/node-ytdl-core and code:
https://pastebin.com/7t6dgSGG

@idk-pixel
Copy link

I don't use discord-plauet. Does that mean I just use ytdl-core-as

@ravindu01manoj
Copy link

I don't use discord-plauet. Does that mean I just use ytdl-core-as

Is it work without this error?

@TimeForANinja
Copy link
Collaborator

Issue with making it a PR is that it's fundamentally changing the way the module works, and I don't know if it's the best way.

There's semantic versioning to take care of this
when there's a good reason for a breaking change we will do it 🤷

there's no confirmation emitted on success, so there's no way to know that it was successful.

there's the format event that get's emitted when getInfo worked and some functions to choose a matching format ran (those shouldn't fail though - would have to check the source code to 100% confirm this though)

passes: 3,

no idea what that parameter is supposed to do - you pass it do ytdl_as as option 🤔


An idea you might consider: the getInfo and downloadFromInfo functions are exposed when requiring ytdl-core
instead of modifying the ytdl default function AND the way you use it, you might just use those methods

if (["youtube"].includes(track.raw.source)) {
  ytdl.getInfo(track.url).then(info => {
    let stream = ytdl.downloadFromInfo(info, { highWaterMark: 1 << 27 });
    // start playing stream and other stuff
  }).catch(err => {
    // send message that it didn't work and other stuff
  })
}

@Tejas12972
Copy link

still receiving the status code 403 while still using this version unfortunately

@Nyako01
Copy link

Nyako01 commented Nov 11, 2021

i got error code 410 when using getBasicInfo(). and this happens in all videos. but lucky me. This problem has been solved by changing the hosting region [heroku] from US to Europe. but sometimes i get error 403
does ytdl get region lock in US?

btw. how to get the error code data in getBasicInfo.catch() and store to variable?

I want to make a kind of switch..case for a specific error code and execute code based on that error code
example:

ytdl.getBasicInfo(url).then(data => {
  //do something
}).catch(err => {
  switch (err) {
    case 410:
      //do something
      break;
    case 403:
      //do something
      break;
    case 404:
      //do something
      break;
  }
})

@idk-pixel
Copy link

idk-pixel commented Nov 13, 2021 via email

@Nyako01
Copy link

Nyako01 commented Nov 13, 2021

e = error

Okay. i think it will be like const error = e.errcode

@idk-pixel
Copy link

@Nyako01 nope
error.code

@TimeForANinja
Copy link
Collaborator

closing since #1022 was merged

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

No branches or pull requests

8 participants