Skip to content

Same thread executor #5072

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Same thread executor #5072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cea98f
Create SameThreadExecutorService for use by tests
Oct 13, 2025
a874bd1
Make AbstractDiscoveryService more overridable
Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 41 additions & 7 deletions ...g.discovery/src/main/java/org/openhab/core/config/discovery/AbstractDiscoveryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.openhab.core.i18n.TranslationProvider;
import org.openhab.core.thing.ThingTypeUID;
import org.openhab.core.thing.ThingUID;
import org.openhab.core.util.SameThreadExecutorService;
import org.osgi.framework.Bundle;
import org.osgi.framework.FrameworkUtil;
import org.slf4j.Logger;
Expand All @@ -59,24 +60,24 @@ public abstract class AbstractDiscoveryService implements DiscoveryService {

private final Logger logger = LoggerFactory.getLogger(AbstractDiscoveryService.class);

protected final ScheduledExecutorService scheduler = ThreadPoolManager.getScheduledPool(DISCOVERY_THREADPOOL_NAME);
protected final ScheduledExecutorService scheduler;

private final Set<DiscoveryListener> discoveryListeners = new CopyOnWriteArraySet<>();
protected final Set<DiscoveryListener> discoveryListeners = new CopyOnWriteArraySet<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Nadahar Nadahar Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// All access must be guarded by "this"
protected @Nullable ScanListener scanListener;

private volatile boolean backgroundDiscoveryEnabled;

private final @Nullable String scanInputLabel;
private final @Nullable String scanInputDescription;
protected final @Nullable String scanInputLabel;
protected final @Nullable String scanInputDescription;

// All access must be guarded by "cachedResults"
private final Map<ThingUID, DiscoveryResult> cachedResults = new HashMap<>();
protected final Map<ThingUID, DiscoveryResult> cachedResults = new HashMap<>();

// This set is immutable and can safely be shared between threads
private final Set<ThingTypeUID> supportedThingTypes;
private final int timeout;
protected final Set<ThingTypeUID> supportedThingTypes;
protected final int timeout;

// All access must be guarded by "this"
private Instant timestampOfLastScan = Instant.MIN;
Expand Down Expand Up @@ -107,6 +108,39 @@ protected AbstractDiscoveryService(@Nullable Set<ThingTypeUID> supportedThingTyp
if (timeout < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new IllegalArgumentException("The timeout must be >= 0!");
}
this.scheduler = ThreadPoolManager.getScheduledPool(DISCOVERY_THREADPOOL_NAME);
this.supportedThingTypes = supportedThingTypes == null ? Set.of() : Set.copyOf(supportedThingTypes);
this.timeout = timeout;
this.backgroundDiscoveryEnabled = backgroundDiscoveryEnabledByDefault;
this.scanInputLabel = scanInputLabel;
this.scanInputDescription = scanInputDescription;
}

/**
* Creates a new instance of this class with the specified parameters.
* <p>
* <b>For use by tests only</b>, allows setting a different {@link ScheduledExecutorService} like
* {@link SameThreadExecutorService} for synchronous behavior during testing.
*
* @param scheduler the {@link ScheduledExecutorService} to use.
* @param supportedThingTypes the list of Thing types which are supported (can be null)
* @param timeout the discovery timeout in seconds after which the discovery
* service automatically stops its forced discovery process (>= 0).
* @param backgroundDiscoveryEnabledByDefault defines, whether the default for this discovery service is to
* enable background discovery or not.
* @param scanInputLabel the label of the optional input parameter to start the discovery or null if no input
* parameter supported
* @param scanInputDescription the description of the optional input parameter to start the discovery or null if no
* input parameter supported
* @throws IllegalArgumentException if {@code timeout < 0}
*/
protected AbstractDiscoveryService(ScheduledExecutorService scheduler,
@Nullable Set<ThingTypeUID> supportedThingTypes, int timeout, boolean backgroundDiscoveryEnabledByDefault,
@Nullable String scanInputLabel, @Nullable String scanInputDescription) throws IllegalArgumentException {
if (timeout < 0) {
throw new IllegalArgumentException("The timeout must be >= 0!");
}
this.scheduler = scheduler;
this.supportedThingTypes = supportedThingTypes == null ? Set.of() : Set.copyOf(supportedThingTypes);
this.timeout = timeout;
this.backgroundDiscoveryEnabled = backgroundDiscoveryEnabledByDefault;
Expand Down
22 changes: 20 additions & 2 deletions ...scovery/src/test/java/org/openhab/core/config/discovery/AbstractDiscoveryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.openhab.core.i18n.TranslationProvider;
import org.openhab.core.thing.ThingTypeUID;
import org.openhab.core.thing.ThingUID;
import org.openhab.core.util.SameThreadExecutorService;
import org.osgi.framework.Bundle;

/**
Expand Down Expand Up @@ -101,9 +102,11 @@ public Locale getLocale() {

class TestDiscoveryService extends AbstractDiscoveryService {

int discoveryResults;

public TestDiscoveryService(TranslationProvider i18nProvider, LocaleProvider localeProvider)
throws IllegalArgumentException {
super(Set.of(THING_TYPE_UID), 1, false);
super(new SameThreadExecutorService(), Set.of(THING_TYPE_UID), 1, false, null, null);
this.i18nProvider = i18nProvider;
this.localeProvider = localeProvider;
}
Expand All @@ -115,19 +118,22 @@ protected void startScan() {
DiscoveryResult discoveryResult = DiscoveryResultBuilder.create(THING_UID1).withThingType(THING_TYPE_UID)
.withProperties(properties).withRepresentationProperty(KEY1).withBridge(BRIDGE_UID)
.withLabel(DISCOVERY_LABEL).build();
discoveryResults++;
thingDiscovered(discoveryResult);

// Discovered thing 2 has a hard coded label but with a key based on its thing UID defined in the properties
// file => the value from the properties file should be considered
discoveryResult = DiscoveryResultBuilder.create(THING_UID2).withThingType(THING_TYPE_UID)
.withProperties(properties).withRepresentationProperty(KEY1).withLabel(DISCOVERY_LABEL).build();
discoveryResults++;
thingDiscovered(discoveryResult);

// Discovered thing 3 has a label referencing an entry in the properties file and no key based on its thing
// UID defined in the properties file => the value from the properties file should be considered
discoveryResult = DiscoveryResultBuilder.create(THING_UID3).withThingType(THING_TYPE_UID)
.withProperties(properties).withRepresentationProperty(KEY1).withBridge(BRIDGE_UID)
.withLabel(DISCOVERY_LABEL_KEY1).build();
discoveryResults++;
thingDiscovered(discoveryResult);

// Discovered thing 4 has a label referencing an entry in the properties file and a key based on its thing
Expand All @@ -136,15 +142,23 @@ protected void startScan() {
discoveryResult = DiscoveryResultBuilder.create(THING_UID4).withThingType(THING_TYPE_UID)
.withProperties(properties).withRepresentationProperty(KEY1).withLabel(DISCOVERY_LABEL_KEY2)
.build();
discoveryResults++;
thingDiscovered(discoveryResult);
}

@Override
protected void stopScan() {
discoveryResults--;
thingRemoved(THING_UID3);
}
}

class TestDiscoveryServiceWithRequiredCode extends AbstractDiscoveryService {

public TestDiscoveryServiceWithRequiredCode(TranslationProvider i18nProvider, LocaleProvider localeProvider)
throws IllegalArgumentException {
super(Set.of(THING_TYPE_UID), 1, false, PAIRING_CODE_LABEL, PAIRING_CODE_DESCR);
super(new SameThreadExecutorService(), Set.of(THING_TYPE_UID), 1, false, PAIRING_CODE_LABEL,
PAIRING_CODE_DESCR);
this.i18nProvider = i18nProvider;
this.localeProvider = localeProvider;
}
Expand Down Expand Up @@ -192,6 +206,7 @@ public void thingDiscovered(DiscoveryService source, DiscoveryResult result) {

@Override
public void thingRemoved(DiscoveryService source, ThingUID thingUID) {
assertThat(thingUID, is(THING_UID3));
}

@Override
Expand All @@ -208,6 +223,9 @@ public void testDiscoveryResults() {
assertNull(discoveryService.getScanInputDescription());
discoveryService.addDiscoveryListener(this);
discoveryService.startScan();
assertEquals(4, discoveryService.discoveryResults);
discoveryService.stopScan();
assertEquals(3, discoveryService.discoveryResults);
}

@Test
Expand Down
Loading