discord-jda / JDA

Java wrapper for the popular chat & VOIP service: Discord https://discord.com
Apache License 2.0
4.35k stars 734 forks source link

Unknown Channel error by sending messages to deleted channels with no way to check if deleted or not beforehand #1256

Closed simoncools closed 4 years ago

simoncools commented 4 years ago

General Troubleshooting

Bug Report

When queueing a message using TextChannel.sendMessage("").queue(); I get an Unknown Channel Error thrown even though i checked for write permissions to the channel. Meaning I get True returned from TextChannel.hasPermission(...); when applied on a deleted text channel.

Is there maybe a way to check if a text channel has been deleted or not? I couldn't find any way myself.

Expected Behavior

I store text channel id's in a database to post messages at certain intervals. Before sending a message I check for write permissions in order to make sure my bot is able to post in the text channel.

Code Example or Reproduction Steps

String channelID = "abcdefg" //Id of a previously deleted channel here JDA.getTextChannelById(channelID).sendMessage("").queue();

Code for JDABuilder or DefaultShardManagerBuilder Used

DefaultShardManagerBuilder builder = new DefaultShardManagerBuilder(); String token = "XXXXX"; builder.setShardsTotal(10); builder.setToken(token); builder.addEventListeners(new Main()); shardManager = builder.build();

Exception or Error

image

Andre601 commented 4 years ago

Please read the Javadoc next time: https://github.com/DV8FromTheWorld/JDA/blob/82c0615630b39244a671245f876de3afc3f36bb7/src/main/java/net/dv8tion/jda/api/JDA.java#L1226-L1243

getTextChannelById returns a possibly-null TextChannel, meaning it can actually be null, if the TextChannel doesn't exist.

Your code-example shows, that you don't try any checks first for if the channel returns null, before actually sending the message, so this obviously will throw an error when the channel was deleted before. This is in no way a bug and instead intended behaviour.

simoncools commented 4 years ago

It's normal behaviour, as getTextChannelById(String) will return null if it can't get the text channel by the provided ID.

Just simply make an instance of TextChannel using the get method and check if the channel is null. This is in no way a bug and actually intended behaviour.

This is what I would expect, I didn't mention it in the post but the textchannel is not null. I always check for it being null and it passes that test. As i said, i'm even able to check for permissions on the channel even though it has been deleted.

DManstrator commented 4 years ago

I believe you are storing the TextChannel and using that somewhere. Because if you get that channel by id and try to send a message on it you should get a NullPointerException and not that ContextException with UnknownChannel. Also I don't see where you actually check the write permission in the code you have posted. cc @Andre601

Andre601 commented 4 years ago

You run getTextChannelById(String).sendMessage(String).queue(); and even mention that the TextChannel was deleted at that point, so getTextChannelById returns null and the sendMessage method fails because of this. Your code example clearly doesn't show any example of you "checking for it being null" I also updated my original comment with more info.

DManstrator commented 4 years ago

@Andre601 Again, he doesn't get an NPE so that line shouldn't even be triggering that exception. Pretty sure he does the check he mentioned on a hardcoded TextChannel which is why this ContextException is thrown.

simoncools commented 4 years ago

I believe you are storing the TextChannel and using that somewhere. Because if you get that channel by id and try to send a message on it you should get a NullPointerException and not that ContextException with UnknownChannel. Also I don't see where you actually check the write permission in the code you have posted. cc @Andre601

You run getTextChannelById(String).sendMessage(String).queue(); and even mention that the TextChannel was deleted at that point, so getTextChannelById returns null and the sendMessage method fails because of this. Your code example clearly doesn't show any example of you "checking for it being null" I also updated my original comment with more info.

The code i posted was just basic, Here's a more complete overhead of what i'm doing (theres more code in between this but not discord API related) Pardon me if i forgot a bracket somewhere, i deleted all the non-important stuff

String nextChannel = "XXX" //Channel ID from MySQL database
TextChannel textChannel = shardManager.getTextChannelById(nextChannel);
if(textChannel != null) {   
    Guild guild = textChannel.getGuild();
    Member selfMember = guild.getSelfMember();
    if(textChannel.isNSFW() || true) {  //Override this
        if (selfMember.hasPermission(textChannel, Permission.MESSAGE_WRITE)) {
                                //code code code
                textChannel.sendMessage("blabla").queue();
        }
    }
}

The error is thrown at the line 'textChannel.sendMessage("blabla").queue();' As you can see, i check quite a lot of things about the text channel without getting a nullpointer thrown.

simoncools commented 4 years ago

I believe you are storing the TextChannel and using that somewhere. Because if you get that channel by id and try to send a message on it you should get a NullPointerException and not that ContextException with UnknownChannel. Also I don't see where you actually check the write permission in the code you have posted. cc @Andre601

Am not storing the TextChannel object but grabbing it by ID, see code i posted earlier. I really don't know why no NullPointer or anything is being thrown here. It's almost like Discord keeps deleted channels in their API for some time?

Andre601 commented 4 years ago

@Andre601 Again, he doesn't get an NPE so that line shouldn't even be triggering that exception. Pretty sure he does the check he mentioned on a hardcoded TextChannel which is why this ContextException is thrown.

From what I know myself does getTextChannelById not even throw an NPE, or else all the places where I use it followed by a TextChannel == null would not work as the method itself would already throw a NPE(?)

DManstrator commented 4 years ago
RestAction.setPassContext(true); // enable context by default
RestAction.DEFAULT_FAILURE = Throwable::printStackTrace;

Add this to your code after building your bot. After that run you code and check your stacktrace where exactly the exception is thrown.

When you don't store any JDA entity, this really might be a bug then.

Edit:

getTextChannelById not even throw an NPE

Correct but calling a method on null (channel not found) will do. @Andre601

simoncools commented 4 years ago
RestAction.setPassContext(true); // enable context by default
RestAction.DEFAULT_FAILURE = Throwable::printStackTrace;

Add this to your code after building your bot. After that run you code and check your stacktrace where exactly the exception is thrown.

When you don't store any JDA entity, this really might be a bug then.

getTextChannelById not even throw an NPE

Correct but calling a method on null (channel not found) will do. @Andre601

Your code is not compiling for me, problem at RestAction.DEFAULT_FAILURE = Throwable::printStackTrace;

RestAction.setDefaultFailure(Throwable::printStackTrace); Is this what you're trying to do?

DManstrator commented 4 years ago

Is this what you're trying to do?

Then it was updated. But yeah, that should be the same.

simoncools commented 4 years ago

Is this what you're trying to do?

Then it was updated. But yeah, that should be the same.

I'll post a stacktrace when i get to it, was changing some other stuff so gotta get that sorted out first

simoncools commented 4 years ago

Is this what you're trying to do?

Then it was updated. But yeah, that should be the same.

image To me, this really doesn't get me any further. Mabye you can see something wrong

DManstrator commented 4 years ago

Can you give us the code from the lines mentioned from the stacktrace and the one around? That should tell us the problem.

simoncools commented 4 years ago

Can you give us the code from the lines mentioned from the stacktrace and the one around? That should tell us the problem.

I am basically running this method in a seperate thread periodically. This is where the error is thrown. I wrote a comment above the sendMessage where the error is thrown.


  public static int updateSubRedditNew(String subreddit, ArrayList<RedditPost> postList){
        subreddit = subreddit.toLowerCase();
        ArrayList<RedditPost> newPostList = new ArrayList<>();
        Iterator<RedditPost> it = postList.iterator();
        int postcounter = 0;
        while(it.hasNext()) {
            RedditPost nextPost = it.next();
            String permaLink = nextPost.getPermaLink();
            boolean duplicate = false;
            Iterator<RedditPost> newIt = newPostList.iterator();
            while(newIt.hasNext() && !duplicate){
                RedditPost nextNewPost = newIt.next();
                if(nextNewPost.getPermaLink().equals(permaLink)){
                    duplicate = true;
                    System.out.println("Duplicate found");
                }
            }
            if(!duplicate){
                newPostList.add(nextPost);
                postcounter++;
            }
        }
        if(postcounter>0) {
            System.out.println(postcounter + " New posts in r/" + subreddit);
        }
        try{
            ArrayList<String> channelList = database.getChannelsSubscribedToSubreddit(subreddit);
            if(channelList!=null){
                if(channelList.size()==0){
                    System.out.println("No channel subscribed to "+subreddit+". Removing from list");
                    try {
                        database.removeReddit(subreddit);
                    }catch(SQLException e){e.printStackTrace();}
                }else {
                    Iterator<String> channelIterator = channelList.iterator();
                    while (channelIterator.hasNext()) {

                        String nextChannel = channelIterator.next();
                        TextChannel textChannel = shardManager.getTextChannelById(nextChannel);
                        if(textChannel != null) {
                            //  System.out.println("Posting to :"+textChannel.getId());
                            if(textChannel.getSlowmode()==0){
                                //  System.out.println("4");
                                Guild guild = textChannel.getGuild();
                                Member selfMember = guild.getSelfMember();
                                if(textChannel.isNSFW() || true) {
                                    if (selfMember.hasPermission(textChannel, Permission.MESSAGE_WRITE)) {
                                        Iterator<RedditPost> postIterator = newPostList.iterator();
                                        EmbedBuilder eb = new EmbedBuilder();
                                        int maxposts=1;
                                        try{
                                            if(database.GuildHasPremium(guild)){
                                                maxposts=25;
                                            }
                                        }catch(SQLException e){e.printStackTrace();}
                                        int maxPostCounter=0;
                                        while (postIterator.hasNext() && maxPostCounter<maxposts) {
                                            // System.out.println("1");

                                            RedditPost myPost = postIterator.next();
                                            if(!textChannel.isNSFW() && myPost.nsfw){
                                                try {
                                                    System.out.println("Tried to post NSFW post in non NSFW channel");
                                                    textChannel.sendMessage("Post is NSFW, channel is not NSFW. Please mark this channel as NSFW to see NSFW reddit posts").queue();
                                                }catch(InsufficientPermissionException e){
                                                    System.out.println("Cannot write to channel");
                                                }catch (ErrorResponseException e){
                                                    System.out.println("Channel Not Found");
                                                }
                                            }else {
                                                maxPostCounter++;
                                                if ((myPost.getUrl().endsWith(".jpg") || myPost.getUrl().endsWith(".png"))) {
                                                    if (selfMember.hasPermission(textChannel, Permission.MESSAGE_WRITE)) {
                                                        // System.out.println("2");
                                                        //  System.out.println(myPost.getTitle());
                                                        String myTitle = myPost.getTitle() + "(" + subreddit + ")";
                                                        eb.setDescription(myTitle);
                                                        eb.setImage(myPost.getUrl());
                                                        eb.setFooter("https://www.reddit.com" + myPost.getPermaLink(), myPost.getUrl());
                                                        try {
                                                            textChannel.sendMessage(eb.build()).queue();
                                                        } catch (InsufficientPermissionException e) {
                                                            System.out.println("Cannot write to channel");
                                                        }catch (ErrorResponseException e){
                                                            System.out.println("Channel Not Found");
                                                        }
                                                        //  System.out.println("3");
                                                        // System.out.println(myPost.getTitle());
                                                    } else {
                                                        System.out.println("Cannot talk in channel");
                                                    }
                                                } else {
                                                    //  System.out.println("3");
                                                    // System.out.println(myPost.getTitle());
                                                    String selftext = "\n" + myPost.getSelftext();
                                                    if (selftext.length() >= 200) {
                                                        selftext = "\nClick link to read post.";
                                                    }
                                                    if (myPost.getUrl().equals("https://www.reddit.com" + myPost.getPermaLink())) {
                                                        try {

                                                            /*

                                                            ERROR IS THROWN BY THIS SENDMESSAGE

                                                             */
                                                            textChannel.sendMessage("-----\n" + myPost.getTitle() + "\n" + myPost.getUrl() + selftext).queue();
                                                        } catch (InsufficientPermissionException e) {
                                                            System.out.println("Cannot write to channel");
                                                        }catch (ErrorResponseException e){
                                                            System.out.println("Channel Not Found");
                                                        }

                                                    } else {
                                                        try {
                                                            textChannel.sendMessage("-----\n" + myPost.getTitle() + "\n" + myPost.getUrl() + selftext + "\n" + "<https://www.reddit.com" + myPost.getPermaLink() + ">").queue();

                                                        } catch (InsufficientPermissionException e) {
                                                            System.out.println("Cannot write to channel");
                                                        }catch (ErrorResponseException e){
                                                            System.out.println("Channel Not Found");
                                                        }
                                                    }
                                                }
                                            }
                                            try{
                                                Thread.sleep(30);
                                            }catch(InterruptedException e){

                                            }
                                        }
                                    } else {
                                        System.out.println("Cannot write to channel");
                                    }

                                }else{
                                    //database.unsubscribe(subreddit,textChannel);
                                    try {
                                        textChannel.sendMessage("Unsubscribed from "+subreddit+". Channel is not marked NSFW").queue();

                                    }catch(InsufficientPermissionException e){
                                        System.out.println("Cannot write to channel");
                                    }
                                }
                            }else{
                                System.out.println("Slowmode on "+textChannel.getId());
                                try{
                                    database.unsubscribe(subreddit,textChannel);
                                }catch(SQLException e){e.printStackTrace();}
                                try {
                                    textChannel.sendMessage("Unsubscribed from "+subreddit+". Please disable Slowmode on this channel.").queue();

                                }catch(InsufficientPermissionException e){
                                    System.out.println("Cannot write to channel");
                                }

                            }

                        }else{
                            //System.out.println("Cannot find text channel, Unsubscribing");
                        }
                    }
                }
            }
        }catch(SQLException e){e.printStackTrace();}
        //  System.out.println("5");
        return 1;
    }
DManstrator commented 4 years ago

And what exactly are the lines mentioned in the stacktrace? Had to tell it from the amount of lines here.

simoncools commented 4 years ago

And what exactly are the lines mentioned in the stacktrace? Had to tell it from the amount of lines here.

Line 171 in Main is this method being called. Line 760 in Main is the line under the annotation EDIT : All the other lines mentioned are not my code


 /*                                                        
ERROR IS THROWN BY THIS SENDMESSAGE                                                            
*/
textChannel.sendMessage("-----\n" + myPost.getTitle() + "\n" + myPost.getUrl() + selftext).queue();
MinnDevelopment commented 4 years ago

Checking whether the channel is deleted is not something JDA supports. It is ideal to not rely on a check like this since it will introduce a TOCTOU Race-Condition.

You can handle the UNKNOWN_CHANNEL error by passing a failure callback to the queue(Consumer, Consumer) method:

channel.sendMessage(content).queue(null, new ErrorHandler()
    .handle(ErrorResponse.UNKNOWN_CHANNEL, (exception) -> {
        // code to handle exception here
    }));

See Also: ErrorHandler

Keep in mind, the JDA cache does not represent the actual state of the discord api and is always impacted by cache update latency. The cache might still contain an entity that was deleted because of the gateway delay (impacted by the time of the event listeners and bandwidth). It is always possible that you encounter race-conditions where the cache falls behind.

You cannot catch an ErrorResponseException because it happens in another thread due to the asynchronous nature of the queue rate-limit handling. These exception can only be handled by passing the failure callback.

DManstrator commented 4 years ago

I believe the problem is that you define the TextChannel and check it being valid and then you use it in a while loop without checking it anymore. In the meantime it got deleted and you are still in the while loop.

You could listen to TextChannelDeleteEvent and log if it was deleted and log when you enter and leave the loop. Pretty sure the event is fired in between leading to this.

simoncools commented 4 years ago

I believe the problem is that you define the TextChannel and check it being valid and then you use it in a while loop without checking it anymore. In the meantime it got deleted and you are still in the while loop.

You could listen to TextChannelDeleteEvent and log if it was deleted and log when you enter and leave the loop. Pretty sure the event is fired in between leading to this.

I really doubt this is the case here. This while loop takes <100ms to finish. 90% of the time it only does one iteration and no more than 25 iterations. And the error pops up very frequently.

MinnDevelopment commented 4 years ago

The code you have provided cannot be used to consistently reproduce the issue. I am unable to fix something I cannot reproduce.

DManstrator commented 4 years ago

Did you verify that or did you just guess? Because I can't see another reason here tbh. Try to debug / log things and see it. The ContextException has nothing to do with missing permissions, the channel just got deleted. If it doesn't exist, it should return null and you already handle that. The channel gets unavailable before you leave that loop or before the request is actually executed.

MinnDevelopment commented 4 years ago

The channel gets unavailable before you leave that loop or before the request is actually executed.

That is not necessarily the case. The requests are only added to the request queue and will execute later. It is possible that they even got rate limited and execute a few seconds after the loop stopped.

simoncools commented 4 years ago

The code you have provided cannot be used to consistently reproduce the issue. I am unable to fix something I cannot reproduce.

There's not much more I can give you to be honest. All i'm doing is storing text channel ID's at one point in time, and using getTextChannelByID to get a TextChannel object and send a message. Somehow the API still returns the normal TextChannel object even though it's been deleted (according to the error i'm getting)

simoncools commented 4 years ago

The channel gets unavailable before you leave that loop or before the request is actually executed.

That is not necessarily the case. The requests are only added to the request queue and will execute later. It is possible that they even got rate limited and execute a few seconds after the loop stopped.

Then again, i've been seeing this error too many times for it to be people deleting text channels within the span of even a few seconds

MinnDevelopment commented 4 years ago
void test(JDA jda) {
    jda.awaitReady();

    val channel = jda.getTextChannelById(id);
    for (int i = 0; i < 10; i++) {
        channel.sendMessage("Hello friend " + i).queue();
    }
    channel.delete().queue();
}

This code will consistently get this issue. The reason is that the channel.delete().queue() runs in parallel to the sendMessage(..).queue() due to both being in seperate queues. There is no way to check and avoid this problem because you are dealing with asynchronous requests that will run in parallel.

The channel is deleted after the loop finishes but the requests are executed after the delete happened thus you get an UNKNOWN_CHANNEL response from the discord api.

With a slightly modified version of the above code you can check that JDA properly handles and updates the cache when a channel is deleted:

void test(JDA jda) {
    new Thread(() -> {
      jda.awaitReady();

      val channel = jda.getTextChannelById(id);
      channel.delete().queue();
      Thread.sleep(5000);
      System.out.println(jda.getTextChannelById(id) == null); // true
    }).start(); // to avoid blocking the event thread
}

In my testing, I consistently get the correct output on this code sample. I have now provided 2 reproducible code samples.

simoncools commented 4 years ago
void test(JDA jda) {
    jda.awaitReady();

    val channel = jda.getTextChannelById(id);
    for (int i = 0; i < 10; i++) {
        channel.sendMessage("Hello friend " + i).queue();
    }
    channel.delete().queue();
}

This code will consistently get this issue. The reason is that the channel.delete().queue() runs in parallel to the sendMessage(..).queue() due to both being in seperate queues. There is no way to check and avoid this problem because you are dealing with asynchronous requests that will run in parallel.

The channel is deleted after the loop finishes but the requests are executed after the delete happened thus you get an UNKNOWN_CHANNEL response from the discord api.

With a slightly modified version of the above code you can check that JDA properly handles and updates the cache when a channel is deleted:

void test(JDA jda) {
    new Thread(() -> {
      jda.awaitReady();

      val channel = jda.getTextChannelById(id);
      channel.delete().queue();
      Thread.sleep(5000);
      System.out.println(jda.getTextChannelById(id) == null); // true
    }).start(); // to avoid blocking the event thread
}

In my testing, I consistently get the correct output on this code sample. I have now provided 2 reproducible code samples.

This might indeed be what is happening. But i'm wondering how slow this channel delete queue is? I've been seeing the error for the same text channel for over 10 minutes.

MinnDevelopment commented 4 years ago

wondering how slow this channel delete queue is

I'm not sure what you mean or how that is related. The issue is a race-condition that cannot be checked beforehand and the solution is to handle the failure with the callback as I have described above. I don't think this is a bug in JDA but rather an issue in your handling with asynchronous operations and expectations of the cache state.

simoncools commented 4 years ago

wondering how slow this channel delete queue is

I'm not sure what you mean or how that is related. The issue is a race-condition that cannot be checked beforehand and the solution is to handle the failure with the callback as I have described above. I don't think this is a bug in JDA but rather an issue in your handling with asynchronous operations and expectations of the cache state.

Actually, i've just looked at your code again and am not sure if this tackles the problem i'm having. I myself am not deleting any channels. I'm solely sending messages to them. So i can't possibly know if a channel has been deleted or not by it's ID alone since it returns a regular TextChannel object. Maybe you thought I was using TextChannel.delete() as well?

PS : The method i posted earlier is running in a seperate thread like you are doing in your code

MinnDevelopment commented 4 years ago

I'm talking about this code:

channel.sendMessage(content).queue(null, new ErrorHandler()
    .handle(ErrorResponse.UNKNOWN_CHANNEL, (exception) -> {
        // code to handle exception here
    }));

As I've said multiple times now, you cannot check whether the channel is deleted before sending. I'm only trying to explain situations where it would occur and that it is unavoidable. It comes down to you having to handle the exception.

simoncools commented 4 years ago

I'm talking about this code:

channel.sendMessage(content).queue(null, new ErrorHandler()
    .handle(ErrorResponse.UNKNOWN_CHANNEL, (exception) -> {
        // code to handle exception here
    }));

As I've said multiple times now, you cannot check whether the channel is deleted before sending. I'm only trying to explain situations where it would occur and that it is unavoidable. It comes down to you having to handle the exception.

Oh okay my bad! I misunderstood then. I've added that code and it is in fact handling the exceptions.

Been breaking my head over this error all week. Thanks for clearing this up

MinnDevelopment commented 4 years ago

Closing, since this seems to be resolved now. If you have any reproducible bugs to report, feel free to open a new issue with the reproduction steps.