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

Force immediate shutdown #1141

Closed
3 tasks done
mlnrDev opened this issue Nov 18, 2019 · 11 comments
Closed
3 tasks done

Force immediate shutdown #1141

mlnrDev opened this issue Nov 18, 2019 · 11 comments

Comments

@mlnrDev
Copy link
Contributor

mlnrDev commented Nov 18, 2019

General Troubleshooting

  • I have checked for similar issues.
  • I have updated to the latest JDA version.
  • I have checked the branches or the maintainers' PRs for upcoming features.

Feature Request

So, i asked about this "feature" (more like an addition or fix if you wish) in the JDA guild already and i was advised to rather create an issue instead of creating a PR, so here it is. I saw many people having "issue" with JDA not shutting down immediately even when using shutdownNow() method. This could be fixed by using two methods provided in the http client.

Example Use-Case

These are the methods that could result in an immediate shutdown.

httpClient.connectionPool().evictAll();
httpClient.dispatcher().executorService().shutdown(); //assuming httpClient is really an http client variable, of course

Those methods could be implemented in 2 ways.

  1. Implement them into the shutdownNow() method
  2. Create a separate method for it, for example shutdownForcely(), which would call shutdownNow() and those 2 methods

Example

  1. shutdownNow()
    @Override
    public synchronized void shutdownNow()
    {
        shutdown();
        threadConfig.shutdownNow();
        this.getHttpClient().connectionPool().evictAll();
        this.getHttpClient().dispatcher().executorService().shutdown();
    }
  1. shutdownForcely() (?)
    @Override
    public synchronized void shutdownForcely()
    {
        shutdownNow();
        this.getHttpClient().connectionPool().evictAll();
        this.getHttpClient().dispatcher().executorService().shutdown();
    }

Calling such methods would make JDA completely unusable for further use, obviously.

@kantenkugel
Copy link
Collaborator

A few things to consider here:

  • We have to be careful to not close clients that JDA didn't create, but were passed into the JDABuilder
  • Being double-careful that those connection pools are per-client and not global OkHttp pools, in which case they would also violate first point
  • can really only be applied to the shutdownNow or a new one like suggested above, since this will abort any ongoing or queued RestActions
  • IMO this is a bug of OkHttp and not sure if adding (possibly temporary) workarounds into JDA is the right call
  • disabling HTTP2 in Okhttp might be the better option, if possible

@mlnrDev
Copy link
Contributor Author

mlnrDev commented Nov 18, 2019

* can really only be applied to the shutdown**Now** or a new one like suggested above, since this will abort any ongoing or queued RestActions

Well, it's obvious that an immediate shutdown is really immediate, so queued RestActions won't get executed and people have to count on it.

* IMO this is a bug of OkHttp and not sure if adding (possibly temporary) workarounds into JDA is the right call

If this is a bug, then it's a really old one, as there were few releases which didn't fix it (yet?). But i agree with other points.

@MinnDevelopment
Copy link
Member

as there were few releases which didn't fix it (yet?).

The same applies to JDA. I have known about this behavior for months if not years and decided not to change our handling. I intentionally did not add any special shutdown behavior for this and I don't intend to do.

@mlnrDev
Copy link
Contributor Author

mlnrDev commented Nov 19, 2019

 * and decided not to change our handling. I intentionally did not add any special shutdown behavior for this and I don't intend to do.

May i please know why?

@MinnDevelopment
Copy link
Member

I consider this bad behavior and a bug in okhttp that we should not need to workaround. I try not to implement workarounds if possible. And in this case its definitely not needed as the thread times out either way.

@robinfriedli
Copy link

Just throwing in my two cents but I would like a blocking shutdown method that only returns once pending RestActions have completed, after that I don't really care about the okhtttp connection pool and could just exit manually. Even with http 1.1 the okhttp still seems to take 1 minute to shut down (considerably better than 5 minutes with http 2 but still).

@mlnrDev
Copy link
Contributor Author

mlnrDev commented Nov 19, 2019

I consider this bad behavior and a bug in okhttp that we should not need to workaround. I try not to implement workarounds if possible.

I mean, the method as shutdownForcely() for example could be useful for people that really want to shut JDA down immediately after calling the method. They might/might not have enough experience to find the solution out by themself.

@gpluscb
Copy link
Contributor

gpluscb commented Nov 19, 2019

I think adding a section in the troubleshooting page of the wiki where the issue and workarounds are explained and a corresponding tag for the Discord servers Spectra bot would be a good option to consider in the case that no changes to JDAs code are made.

@MinnDevelopment
Copy link
Member

I mean, the method as shutdownForcely() for example could be useful for people that really want to shut JDA down immediately after calling the method. They might/might not have enough experience to find the solution out by themself.

From my experience, most people figure out how to use System.exit(0); on their own. I don't really see this as a good argument and adding a bunch of shutdown methods is not useful. Again this would be trying to work around an issue with okhttp which I don't want to do.

adding a section in the troubleshooting page of the wiki where the issue and workarounds are explained

I think that is reasonable.

@MinnDevelopment
Copy link
Member

I have added an entry for this in the troubleshooting guide: Shutdown but the process doesn't exit

@mlnrDev
Copy link
Contributor Author

mlnrDev commented Nov 24, 2019

I have added an entry for this in the troubleshooting guide: Shutdown but the process doesn't exit-Troubleshooting#shutdown-but-the-process-doesnt-exit)

I consider this as a reason to close this issue. Thanks for your time.

@mlnrDev mlnrDev closed this as completed Nov 24, 2019
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

5 participants