JDA icon indicating copy to clipboard operation
JDA copied to clipboard

Add blocking awaitShutdown

Open Kittencore2608 opened this issue 2 years ago • 4 comments

Pull Request Etiquette

Changes

  • [X] Internal code
  • [X] Library interface (affecting end-user code)
  • [ ] Documentation
  • [ ] Other: _____

Closes Issue: NaN

Description

Adds an awaitShutdown() method to JDA for those who wish to block instead of waiting on an async ShutdownEvent for taking actions after JDA shuts down. Internally refactors setStatus/getStatus to be properly threadsafe and sync on a shared object (which I believe should not break existing semantics) which is used to implement the shutdown-await behavior.

Kittencore2608 avatar Dec 24 '21 20:12 Kittencore2608

This may also permit refactoring of awaitStatus using the new monitors

freya022 avatar Dec 24 '21 21:12 freya022

This doesn't really make sense because even when this awaitShutdown returns, JDA might still send out events, do requests, etc. This isn't really going to do what you expect it to do. I also dislike that you introduce some arbitrary object to exploit as object monitor, we already have a working system for this in awaitStatus (which is currently limited to login statuses).

MinnDevelopment avatar Dec 26 '21 16:12 MinnDevelopment

Understandable. I'm hesitant to consider overloading awaitStatus since I'm rather uncertain of why the get/set/awaitStatus manage to work despite not having paired synched read/writes or any obvious threading primitive attached. I'll have to grok the shutdown sequence more to see if/how I can salvage this.

Kittencore2608 avatar Dec 26 '21 21:12 Kittencore2608

Used the volatile modifier for the status flag and went for awaiting the executors and the WS client firing the shutdown event instead, so this should provide semantics more like what I think users would expect it to do.

Kittencore2608 avatar Dec 28 '21 01:12 Kittencore2608

I am choosing to abandon this PR due to lack of continued interest from myself, the fact that spin-on-getStatus has become the de-facto convention for implementing similar behaviour rendering this PR somewhat redundant, and a lack of a good way to obtain the clean-shutdown semantics that I originally hoped for.

Given that the use-case I originally envisioned has been worked around, this PR is not really needed anymore, though anyone who expresses interest and is willing to save it is welcome to do so on their own.

Kittencore2608 avatar Oct 05 '22 19:10 Kittencore2608