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

New feature: request upcoming chimes #30

Open
pmonks opened this issue Jun 29, 2020 · 17 comments
Open

New feature: request upcoming chimes #30

pmonks opened this issue Jun 29, 2020 · 17 comments
Assignees

Comments

@pmonks
Copy link

pmonks commented Jun 29, 2020

It would be useful to be able to ask chime for upcoming scheduled jobs. While my code could cache this information itself, I figure chime is already storing this information somewhere, so it would be better to just ask the source, rather than duplicating it in my own code.

@jarohen
Copy link
Owner

jarohen commented Jun 29, 2020

Hey @pmonks,

You're right, we do have that information, although only as part of this closure: https://github.com/jarohen/chime/blob/master/src/chime/core.clj#L88 - we'd have to change this to be shared state between the caller's thread and the scheduling thread in order to return it as part of the return value from chime-at. I'm not opposed to this, but will have a think of other ways too (suggestions welcome!)

James

jarohen added a commit that referenced this issue Jul 12, 2020
not 100% on this API because they're not accessible from within the scheduling fn - but we can't make them accessible easily without changing the arity of the schedule fn
jarohen added a commit that referenced this issue Aug 17, 2020
not 100% on this API because they're not accessible from within the scheduling fn - but we can't make them accessible easily without changing the arity of the schedule fn
@pmonks
Copy link
Author

pmonks commented Sep 19, 2020

I might be misunderstanding your proposed solution, but what I'd envisaged was a new public fn in the chime.core ns that returns a representation of all upcoming chimes, containing both the (lazy) sequence of upcoming executions and some identifier of the fn that will be executed.

Returning a value from chime-at may also be useful, but that wouldn't directly support my use case (i.e. being able to arbitrarily query chime to find out what jobs are upcoming and when).

@jarohen
Copy link
Owner

jarohen commented Dec 26, 2020

Ah, I see. Assuming I've understood you correctly, this is less appealing, I'm afraid. Chime doesn't store any information globally at the moment - doing so would mean that Chime would either risk leaking memory, or take on the responsibility of managing/cleaning up references - so it's something I'd rather avoid.

@jimpil
Copy link
Contributor

jimpil commented Dec 26, 2020

@pmonks A stateful scheduler which keeps track of active jobs (id => cancel-fn) is pretty trivial to do on-top of chime. In particular you can leverage the :on-finished callback to have your scheduler forget jobs when they're done (cancelled or finished). If I end up forking this project, i will consider officially adding something like this on top of agents.

@pmonks
Copy link
Author

pmonks commented Dec 28, 2020

@jarohen something is already keeping track of all of this information, in order to actually run the jobs at the scheduled times - my hope was simply that chime could provide a way to interrogate that existing data store. To @jimpil's point, I'd much rather not have to duplicate that information again in "user" space.

@jimpil
Copy link
Contributor

jimpil commented Dec 29, 2020

@pmonks chime doesn't keep track of any such information. It may be illuminating to have a look at the source, but chime is essentially a lazy scheduling loop. You provide a 1-arg callback + some times, and chime kicks off a loop which schedules the next chime as soon as the current one finishes. In other words, it doesn't assign ids to jobs, and it doesn't know how many chimes will eventually be run. Moreover, every-time you call chime-at it starts a brand new scheduling loop, which knows nothing about other scheduling loops.

What you want (as I understand it), is a higher-level scheduler construct which can accept multiple scheduling jobs, and at any given time you can ask it for the actively-running jobs (not finished, nor cancelled). The construct I'm using expects something like the following:

;; job-id => job-details
{:foo [foo-times-fn foo-chime-fn]
 :bar [bar-times-fn bar-chime-fn]}

It calls chime-at as usual, but also makes sure to forget ids of jobs that have finished or cancelled.

@jarohen
Copy link
Owner

jarohen commented Dec 29, 2020

@pmonks: @jimpil is right, we don't keep track of what jobs are active. If it helps, it's the same as the underlying Java ExecutorService - the framework doesn't keep references to the pools, it passes the reference (and the responsibility for cleanup) back to the caller. If the caller wants to keep state of all the pools they've created, they can - indeed, they're best placed to.

Regarding upcoming chimes - we could expose more information that we currently do about an individual schedule. Again, @jimpil is right, Chime only knows about the next time a schedule will be called, because it expects the caller to have passed a lazy sequence, and we don't want to evaluate more of that lazy sequence than we need to - it may be infinite, after all.

With the combination of the caller keeping track of the references, and Chime being more helpful about each individual reference, we might be able to provide what you're looking for?

@pmonks
Copy link
Author

pmonks commented Dec 30, 2020

So my code has a daily job (created via chime-at and scheduled to run at midnight UTC every day) that hits a 3rd party data source, and then creates many (potentially up to several hundred or so) "one-shot" jobs during the following 24 hour period, based on the data returned by that data source. The number and timing of these one-shot jobs is unpredictable, and as the name suggests they each run just once.

While my code maintains a reference to the daily job, it discards the response from chime-at for all of the one-shot jobs that are created.

Based on your explanations, this raises a couple of questions:

  1. how are the one-shot jobs not being GCed, given the references to them are discarded?
  2. if the JVM is maintaining those references somewhere, is that "data store" queryable, and could chime simply expose that information?

@jarohen
Copy link
Owner

jarohen commented Dec 30, 2020

Based on your explanations, this raises a couple of questions:

  1. how are the one-shot jobs not being GCed, given the references to them are discarded?

If I understand correctly, it's the underlying JVM Threads that are the GC roots here - the Thread references the ExecutorService's queue, which contains the Chime Runnables.

  1. if the JVM is maintaining those references somewhere, is that "data store" queryable, and could chime simply expose that information?

Not that I know of, I'm afraid.

Would there be anything preventing you from keeping the chime-at responses? An atom containing a map from the match key to the schedule, perhaps?

@jarohen
Copy link
Owner

jarohen commented Dec 30, 2020

Migrated from #37, @jimpil


This is kind of neat and it also kind of addresses #30, if I'm understanding the request correctly:

ScheduledFuture
(cancel [_ interrupt?] ;; expose interrupt facilities (opt-in)
  (when-let [^ScheduledFuture fut @current]
    (and (not (.isCancelled fut))
         (.cancel fut interrupt?))))
(getDelay [_ time-unit] ;; expose remaining time until next chime (partly addresses #30)
  (when-let [^ScheduledFuture fut @current]
    (.getDelay fut time-unit)))

Re ScheduledFuture. I like the idea in theory - in practice it seems to have a couple of tradeoffs?

  • getDelay - would it be preferable to have an API that returns the next time, and let the user calculate the delay, if need be? By the time we've returned the delay, it's already out of date.
  • cancel - this introduces the question of whether I'm cancelling just the current job or not, potentially? I mean, this is potentially a use-case - 'I'd like to cancel the current job but not the overall schedule' (indeed, a question you mention further up). I'm also not mad keen on boolean params in cases like these, I always have to look them up - which is why I'm drawn to something like shutdown/shutdownNow. The word 'shutdown' (to me, at least) is a clearer indication that we're shutting down the whole thing.

Maybe this could be a chime-specific protocol, but with semantics/naming taken from ExecutorService and friends?

My understanding from the Java docs is that [getDelay] is calculated on-the-fly every time you call it, so it's always correct. That said, I would need to look at the actual source to be 100% sure. To be clear, I'm not opposing what you suggested - I'm just saying that there is a good chance this does exactly what a consumer might want (to know how long until the next chime).

It's always correct, but only at the exact moment it returns it's calculated - after that, it's out of date, and the user doesn't know the 'now' basis that was used in the calculation. If we pass the raw date back to the user instead, they're in charge of choosing a fixed 'now' to make that calculation.

After all, he/she does have access to the times (he/she provided them), and there is a utility fn (without-past-times) to use against them if someone wants to find out all the upcoming chime times.

We've fixed an issue previously (#10) around holding on to the head of the lazy seq - I wouldn't want to ask the user to do the same, especially if it's infinite. We have the tail of the sequence available, and could expose it - this would be particularly suitable if the user only wanted the next time, as it wouldn't require evaluating any more of the sequence.

@jimpil
Copy link
Contributor

jimpil commented Dec 31, 2020

@pmonks I am going to try to complement @jarohen 's answers - hope this helps you.

how are the one-shot jobs not being GCed, given the references to them are discarded?
The references that are being discarded are new objects (that implement AutoCloseable). These are not the actual jobs though - these are scheduled (somewhat lazily) on an ExecutorService. Given your description of what the code does, I'd say that these AutoCloseable objects returned by chime-at are being GC'ed.

if the JVM is maintaining those references somewhere, is that "data store" queryable, and could chime simply expose that information?

chime is a little thin (yet powerful) layer on top of vanilla Java single-thread Executor. Every single time you call chime-at, a new single-thread Executor is created and it becomes responsible to schedule your job (one schedule at a time). That is it... There is no 'data-store', certainly not a queryable one :).

By the way, for tens/hundreds/thousands of one-off jobs I'd use future if I could.

@pmonks
Copy link
Author

pmonks commented Dec 31, 2020

If I understand correctly, it's the underlying JVM Threads that are the GC roots here - the Thread references the ExecutorService's queue, which contains the Chime Runnables.

  1. if the JVM is maintaining those references somewhere, is that "data store" queryable, and could chime simply expose that information?

Not that I know of, I'm afraid.

If this information ends up encoded into threads, then those can be enumerated via ThreadMXBean. From there it should be straight forward to narrow the set down to just the chime-created threads, and pull the information out of them (perhaps storing metadata in those threads at creation time, if the information isn't currently encoded in a convenient form in the thread object itself).

Would there be anything preventing you from keeping the chime-at responses? An atom containing a map from the match key to the schedule, perhaps?

Sure, but how would that support the case where some 3rd party code I'm using also uses chime? How would I enumerate all chime jobs across both my own code and that 3rd party code?

Note that this isn't a problem I'm currently facing, but it's not a far-fetched scenario imho.

@jarohen
Copy link
Owner

jarohen commented Dec 31, 2020

Ah, interesting, I hadn't considered JMX. Not sure I'd personally make application logic depend on it? (Obviously not my place to say 'don't', but just not something I've done.)

Sure, but how would that support the case where some 3rd party code I'm using also uses chime? How would I enumerate all chime jobs across both my own code and that 3rd party code?

Note that this isn't a problem I'm currently facing, but it's not a far-fetched scenario imho.

Maybe you could expand on why you might want to know what Chimes a third-party has scheduled? Seems it'd either be up to the third-party to expose its schedules to you via its own API, otherwise it'd be a bit of a leaky abstraction?

@pmonks
Copy link
Author

pmonks commented Jan 2, 2021

Maybe you could expand on why you might want to know what Chimes a third-party has scheduled? Seems it'd either be up to the third-party to expose its schedules to you via its own API, otherwise it'd be a bit of a leaky abstraction?

Sure. I use chime in a chatbot, and amongst various chatops features I'd like it to support is to respond to an admin's request to enumerate upcoming jobs for the day. As mentioned above this varies quite wildly, and because the 3rd party data source charges per request, re-querying and regenerating the schedule of daily jobs from there is financially undesirable.

If I were to start using a third party library that also uses chime, I'd like to know about its jobs too, since the production instance of this bot runs on a resource constrained platform (Heroku), and the code is designed to spread out processing throughout the day to stay within those resource constraints as much as possible.

@jarohen jarohen self-assigned this Jan 4, 2021
@pmonks
Copy link
Author

pmonks commented Jan 4, 2021

Oh and regarding:

Ah, interesting, I hadn't considered JMX. Not sure I'd personally make application logic depend on it? (Obviously not my place to say 'don't', but just not something I've done.)

Yeah I think I have much the same feeling about JMX as you do - it's not generally something I consider to be part of the "core" API the JVM exposes. That said, I have used JMX like this in the distant past (albeit not ThreadMXBean) and it's very handy when you need it.

Another concern for me is where JMX is going to end up in the post-Java 9 / Jigsaw modularised world - it's plausible that JMX will be split out into an optional module, and then the question becomes whether it's in chime's best interest to effectively add a dependency on a (non-JVM-core) module. I should note that a cursory round of googling didn't answer the question of where JMX has ended up, post-Jigsaw.

@awb99
Copy link

awb99 commented Mar 5, 2022

I read this thread. And seems not to be so easy to implement this feature.
That said, I am trying to use chime in my app. And the issue I have is that my jobs are not executing.
So for debugging it would be amazing to have a quick way to find out which are the times that chime would run the
next job.

@pmonks
Copy link
Author

pmonks commented Mar 5, 2022

Actually finding chime's threads is trivial:

(require '[clojure.string :as s])
(def tmx (java.lang.management.ManagementFactory/getThreadMXBean))
(filter #(s/starts-with? (.getThreadName %) "chime-") (map #(.getThreadInfo tmx %) (.getAllThreadIds tmx)))

The only missing piece is to store the chime metadata (the lazy sequence of "run-at" times and the fn that will be called at those times) in each of those threads in such a way that it can be read back out.

[edit] and a potentially more useful alternative (since it provides actual Thread objects not ThreadInfos, and doesn't use JMX):

(require '[clojure.string :as s])
(filter #(s/starts-with? (.getName %) "chime-") (.keySet (Thread/getAllStackTraces)))

I think if chime were to use a subclass of Thread that stored references to the function and schedule, the above code could be used to "query" all chime threads, then (with a downcast) to pull out the fn and schedule and return them. This approach would still be "stateless" (chime would not contain any global state), and would instead query state that the JVM already has (i.e. the set of threads in the JVM).

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

4 participants