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

Fix un-catched exception #120

Merged
merged 3 commits into from
Dec 20, 2024
Merged

Fix un-catched exception #120

merged 3 commits into from
Dec 20, 2024

Conversation

Kile
Copy link
Contributor

@Kile Kile commented Dec 18, 2024

Hi there!
Awesome project!

I have started to have an issue a few weeks ago where Apple Music will stop being responsive to pause and skip buttons. I am not sure if this is because of this library, so I looked into it.

I cannot consistently reproduce this issue, but the one time I could, I found an error in the logs at the same time:
image

Closing connection to Discord RPC
Error: (1000) Request has been terminated
Possible causes: the network is offline, Origin is not allowed by Access-Control-Allow-Origin, the page is being unloaded, etc.
    at DiscordIPC.#read (https://deno.land/x/[email protected]/src/conn.ts:180:11)
    at eventLoopTick (ext:core/01_core.js:175:7)
    at async https://deno.land/x/[email protected]/src/conn.ts:49:11

Looking into the code, it seems this.rpc.close(); is trusted to not error and any error coming from it is not catched. So in this PR I added a try block around it. Even if this is not the root cause of my issue, it is still a good idea to catch any errors from this line of code.

While running this code locally, I also noticed that it complains about

�[0m�[1m�[31merror�[0m: unexpected argument '--allow-import' found

  tip: a similar argument exists: '--allow-hrtime'
  tip: to pass '--allow-import' as a value, use '-- --allow-import'

Usage: deno run [OPTIONS] [SCRIPT_ARG]...

so I followed the suggestion and used --allow-hrtime instead. If this is intended or my system is outdated feel free to let me know and I will revert this change. (I had an outdated deno version)

@NextFire
Copy link
Owner

Hi! Can you tell me which deno version are you currently running? (deno -V)

@Kile
Copy link
Contributor Author

Kile commented Dec 19, 2024

deno 1.46.3

@Kile
Copy link
Contributor Author

Kile commented Dec 19, 2024

Ok I see you mentioned deno 2+ is needed, my bad, I will revert the command line argument then

@Kile Kile changed the title Fix command line argument + un-catched exception Fix un-catched exception Dec 19, 2024
music-rpc.ts Outdated
Comment on lines 46 to 54
if (this.rpc.ipc) {
console.log("Closing connection to Discord RPC");
this.rpc.close();
this.rpc.ipc = undefined;
console.log("Attempting to close connection to Discord RPC");
try {
this.rpc.close();
} finally {
console.log("Connection to Discord RPC closed");
this.rpc.ipc = undefined;
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you extract this block to something like this.tryCloseRPC() to avoid having too many nested statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, will do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also put the if statement in that method, let me know if you want it to take it out again

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, LGTM

@NextFire NextFire merged commit dda4a4e into NextFire:main Dec 20, 2024
2 checks passed
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 this pull request may close these issues.

2 participants