[hpprinter] Prevent "handler disposed" warnings on shutdown

[andrewfg]

This PR eliminates the "handler was already disposed" warnings when OH is being shut down.

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

[Cossey]

Hmmm, I wonder if my other bindings have a similar issue since they are designed in the same way. I'll keep an eye out here and see if there's any relevant fixes that can make their way into the others.

[andrewfg]

Hi @Cossey,

I had a dig into your code, and at first glance it looks as though dispose() does indeed properly call cancel() on all your task schedulers. However digging deeper, I think the issue may be that cancel() only removes not yet started task instances from the queue, and does not kill task instances that have already started. And since such tasks are http calls to the printer, it is possible that the printer takes a while to respond.

Furthermore also since the http calls are handled by a jetty client, it is even possible that jetty "magic" might execute "parallel" http call tasks (status, scanner, usage, etc.) serially on each other (??)

Anyway when such a slow/serialized task finally does return successfully, your code calls handler.updateState() which causes the logger error. Or -- even worse -- if such a slow/serialized task were to return with failure then goneOffline() gets called which causes all your schedulers to restart over!!

So I am going to push a commit shortly with a few modifications for you and Kai to look at.

PS I also note that the binding logs "Initializing handler for thing .. takes more than 5000ms" so my commit will fix that too..

[andrewfg]

@Cossey it occurs to me that I might be seeing the logs because my printer has a slower HTTP server than yours (perhaps in particular when the printer goes into sleep)..

[kaikreuzer]

Hey @andrewfg, sorry if I am complaining again. But your solution again seems to fix the symptoms, but not the root cause. Instead of adding code that checks whether dispose had been called and then do nothing, wouldn't it be much better if the code wouldn't run in the first place? I didn't loo at the binding code in detail, but in general it should be possible to interrupt any running task and make it terminate. Am I missing some complexity here?

[andrewfg]

in general it should be possible to interrupt any running task

The problem is that the running task is an HTTP call to the printer's web server waiting to return. I guess that we could just kill the socket. But I did not want to do that, because it might upset the printer and leave it in an unknown state. (And I can only test on my one single printer vs all other users printers). So I just let the HTTP call return in its own good time, but shortcut the processing of the response.

[kaikreuzer]

Alright then, thanks for the explanation.
Let's do it this way.

[kaikreuzer]

I'll wait with the merge for @Cossey's feedback, though.

[Cossey]

So from what I understand, when the scheduled future is cancelled, the thread continues until completed.

The only thing I would like done with the changes is the function names with 0 in them could be renamed maybe, it just seems improper. Maybe they could be prefixed with startClose or startInitialize or something like that? Unless there’s already an established convention Kai?

Otherwise LGTM and ill update my other bindings with the same approach in the upcoming days.

[kaikreuzer]

The only thing I would like done with the changes is the function names with 0 in them could be renamed maybe, it just seems improper.

I agree with that.
There isn't any established convention, but usually we call such methods something like internalClose or similar.

[andrewfg]

when the scheduled future is cancelled, the thread continues until completed

Hi @Cossey a thread scheduler has two elements; a FIFO queue of tasks that it shall execute in the future, and a pool of tasks that are currently actually being run (each in its own separate thread). When the scheduler starts a task instance it removes it from the queue and adds it to the pool of running tasks. The scheduleWithFixedDelay() method is an "auto-magic" tool that tells the scheduler to keep automatically adding the same respective task to its queue continuously at the specified interval. And the cancel() method tells the scheduler to stop the "auto-magic" process of adding the task repetitively to the queue. As a general rule, the cancel() method doesn't kill an instance of the task that had already left the queue and entered the pool of running tasks (i.e. default behavious is "if you have started so I will let you finish"). The JDoc says this..

Attempts to cancel execution of this task.  This attempt will fail if the task has already completed, has already been
cancelled, or could not be cancelled for some other reason. If successful, and this task has not started when cancel()
is called, this task should never run.  If the task has already started, then the mayInterruptIfRunning parameter
determines whether the thread executing this task should be interrupted in an attempt to stop the task.

But even if you call cancel() with mayInterruptIfRunning it may not actually specifically kill the task; but rather it sets the Thread.interrupted flag on the running thread (and/or throw an Interrupted exception), and your code has to check Thread.interrupted (and/or catch the exception), and exit early if it can (but it may anyway decide not to exit if it is being stubborn).

Our case here is (I think) yet more exotic since the scheduled task is calling a Jetty HTTP client, which (I think) calls a scheduled task inside itself; so there is (I think) a scheduled task within a scheduled task..

[andrewfg]

the function names with 0 in them could be renamed

Ok. Will do.

[kaikreuzer]

Thanks everyone!

[andrewfg]

Thanks @kaikreuzer and @Cossey