Same thread executor

[Nadahar]

After #5032 was merged, there has been problems with tests for some add-ons: openhab/openhab-addons#19230 openhab/openhab-addons#19473

Some tests are still disabled as a consequence. The problem is that when discovery result registration became asynchronous, tests' assumptions of when the results have been registered fails. Previously, when the registration was done synchronously, it took much longer before thingDiscovered() returns, so when the test continued, the results were in. Now, the thingDiscovered() call is very quick, because another thread performs the actual registration.

The remedy for many of the tests have been to introduce delays and timeouts, but for some that hasn't been enough. Relying on timeouts and delays is a bad practice anyway, because you make assumptions of the speed of the test runner. If the test runner is slow, like the GitHub runners sometimes are, tests fail if the delay is too short. At the same time, the more you increase the delays, the slower the tests will be to run even on fast runners. There is no "good solution".

To have a better way to deal with situations like this, I've expanded on the approach made by @david-pace here:

https://github.com/openhab/openhab-addons/blob/e585d8e417e20830c017cb8e581f0369729ef1d0/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/devices/bridge/LongPollingTest.java#L131-L198

(I hope that's OK for you @david-pace)

The SameThreadExecutorService is a "fake" executor service that will run everything synchronously in the calling thread. It has some limitations, particularly that it can't schedule executions ahead of time, but it should be a suitable drop-in for "real" executors that can be useful for tests in "asynchronous scenarios". The idea here is to add this to core, so that binding (and other code) can use it, instead of everybody having to reinvent the wheel.

I've also done some modifications to AbstractDiscoveryService to make it more "override friendly", for more testing flexibility. I think that using a lot of private fields in abstract classes is a bad practice in general, they are supposed to be subclassed, but the often extensive use of private can make this very cumbersome and inflexible.

This should be very low-risk, as it's not actually used by anything in core, except for the modified AbstractDiscoveryServiceTest. I've also slightly expanded the tests in AbstractDiscoveryServiceTest since the existing tests failed to fail as a result of #5032, which would have revealed the issue before merge.

[Nadahar]

I'll add that I intend to address the disabled add-on tests once this is merged, using the new SameThreadExecutorService.

[Nadahar]

I was uncertain in which package to place SameThreadExecutorService. Another ExecutorService implementation is placed in org.openhab.core.common, but since this isn't a "real" executor, I opted to put it in org.openhab.core.util. If that's considered wrong, let me know, and I'll move it.

[Nadahar]

@jlaur @lsiepel FYI

[david-pace]

Constructor code is redundant, I would propose to call this(ThreadPoolManager.getScheduledPool(DISCOVERY_THREADPOOL_NAME), ...) here

[Nadahar]

Technically, yes, but I didn't intend to change more than I had to. I added the other constructor specifically for use by tests, and left this one alone.

[david-pace]

Is there a specific requirement to make the fields protected now? If not, I would vote to keep the visibility as "low" as possible until there is a specific reason to change it.

[Nadahar]

If you want to override thingDiscovered() without implementing your own listener registration, you'll need access to it. I've never understood the philosophy of keeping everything private - it just means that you can't override anything the authors didn't specifically make sure you could. In effect, it's an attack on inheritance itself.

It's an abstract class, it's meant to be "a template" to build on, without having to write everything from scratch. A binding doesn't have to use it at all, it can implement the interface from scratch. But, that's a lot of wasted effort. By locking everything down, you basically force such wasted efforts in situations where the "standard logic" doesn't work.

[david-pace]

In conjuction with inheritance, read and write access to fields is normally provided via methods, not fields directly. In most cases, it is not desirable that subclasses get full read and write access to all fields. That can lead to obscure issues, especially if multiple subclasses come into play that modify the state of the fields in an uncoordinated way. Normally you still want to encapsulate certain functionality in the base class, even if it has subclasses.

Usually abstract classes provide some "basic functionality", and subclasses may extend certain parts (not necessarily all parts) of this functionality. The "base functionality" could for example include validation that passed values are correct. In this case, you would want to centralize and encapsulate the validation in a constructor or setter in the super class, and provide a protected (or even public) getter method. Making all members protected breaks the desired encapsulation in such cases. After all, subclasses could now circumvent or re-define validations. This would violate, for instance, the Liskov Substitution Principle.

That being said, in the case of "trivial" getters/setters that have no added functionality like validation, I acknowledge that we might as well just make a field protected. One case where getters/setters are still preferable, is if we need to override them in subclasses for some reason.

Another aspect I want to bring up is that the protected members effectively define the "internal" API of the class (for subclasses), just like the public members define the API for the "outside world". So making those fields protected is effectively an API change, not just for test code, but also for the internal openHAB API. I'm not sure if this is really needed/wanted, especially not for all fields that were changed to protected in the current PR state. I would vote to only change field visibility if there is a good reason for it, and not simply changing the visibility of (nearly) all fields.

Whatever the case, I am not an openHAB core maintainer, so this is just my personal opinion and if the core maintainers are OK with protected fields the current solution is fine 👍 I'm just bringing it up because it's hard to revert once subclasses access the fields directly.

[Nadahar]

Regarding encapsulation, I know that these ideas are often taught in schools and are thus shared by many. I'm not saying that they are never warranted either, but I think they have "take on a life of their own" and become something closer to ideology than anything based in practical considerations. I especially disagree that encapsulation should be the default. I think encapsulation should be used when there's a reason to enforce something, not just "because there's no well argued reason why it shouldn't be".

I think I know why things have "evolved" in this direction: the desire to reduce complexity. For libraries in particular, giving too much "access" to the code can leave little room for changes, because almost everything "become part of the API". I understand it, and I think it's a terribly unfortunate thing that have arisen, where access has been equalled with "official API". This could easily be solved in other ways, like some kind of documentation that states what is the API, in Java you could even use annotations to achieve this.

This doesn't mean that this isn't without downsides, often big ones. I've too often had to not use a library because they have some behavior that doesn't fit my use case, and they have "encapsulated me" out of doing something about it. You see this "all the time", people have to make forks, that have to be maintained, just for relatively minor tweaks. I think this has come to a point where what it's supposed to solve can't defend all the problems it creates. The world don't seem to agree though, but that's nothing new. This is still about libraries though.

When doing the same internally in an application, I think it has even less merit, if you need to change something that could potentially have been "encapsulated away", you can easily take care of that while making the change. At the same time, you can also easily modify the "base class" to accommodate whatever need that arise, so it might not matter that way which way you choose.

With OH and add-ons, I think the situation is somewhere in between. The "internal API" isn't widely used out in the wild, where OH has no way to influence things. Most add-ons are "official" and can be modified to accommodate changes, which is often done. "Non-official" add-ons are mostly left to "fend for themselves", it's their responsibility to adapt to changes. So, the "need" to protect yourself by encapsulating "everything" isn't nearly as strong as with general libraries, which makes the cost/benefit calculus different. The disadvantages become relatively larger.

When you're dealing with an abstract class that implements an interface, all ideas of "enforcement" are out the window. You can't enforce anything, because others are free to make their own implementation of the interface. So, the whole purpose of the abstract class is to reuse code, the prevent the same thing having to be written over and over again. By applying the "ideology of encapsulation" in such situations, you only achieve one thing: less code reuse. This is directly opposite to the purpose of the abstract class, and I see it as more or less purely counter-productive. Even then, I haven't "blindly" made fields protected. I have done it where I see a need for it to make tests work in a good way, exactly because I know that the "encapsulation ideology" have spread far and have many prophets.

I think the whole things is very sad. These ideas weren't originally OO, I'd say that many of the "principles/ideologies" that have developed over the years are directly alien to OO. It's driven by "puritans" that think that OO should be used solely according to some "divine principles" that they have come to believe in. I have seen how OO code have become less and less flexible over the years as a consequence.

OO also have many critics today, which fuels the drive for alternatives, like functional programming. I think this is mostly driven by hate for these ideologies and all the limitations they impose. Many people are fed up with having to follow all these conventions to do simple things, they "want to be free". But, what many fail to see, is that OO isn't the problem, it's the principles that are applied to it that are. I think OO is a great tool, maybe the greatest of them all within programming. But, it's still just a tool, it's not a religion. So, it should be used to achieve your goals, not to satisfy some ideological overlords. If we could go back to using it as a tool, leveraging its strengths where they are and using other approaches where they fit better, there would be no need for this "ideological divide". It would be no need for all the stupid principles that have formed around functional programming either, it's not like that is any less ideological today.

Many probably disagree with this, I'm merely explaining my stance, well aware that I'm not "mainstream". I never am, because I come to my own conclusions, I never just accept those from the outside world. But, this explains why say and do what I say and do, regardless of mainstream ideology.

When it comes to the concrete changes here, I think the "risk" is practically non-existent, and the reward is that it's easier to make well functioning tests. Others might disagree, but this is my suggestion.

[Nadahar]

@holgerfriedrich It would be nice if this could be reviewed soonish, because I also need to address the disabled tests in addons once this has been merged. The "risk" from this should be non-existent, it can't really break anything.