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

EmbeddedDBusDaemon thread never ends. #222

Closed
brett-smith opened this issue Jun 28, 2023 · 4 comments
Closed

EmbeddedDBusDaemon thread never ends. #222

brett-smith opened this issue Jun 28, 2023 · 4 comments

Comments

@brett-smith
Copy link
Contributor

brett-smith commented Jun 28, 2023

If an EmbeddedDBusDaemon is created and started, then a call to EmbeddedDBusDaemon.close() will not actually stop the thread. Depending on if client connections were made, other threads may be left running.

We noticed this in our application where long running instances of the daemon were building up many threads. Because we are using Unix Domain Sockets, and the domain socket file is being stored is being stored in temporary directory, we found that on some systems (Mac OS iirc), the domain socket file could be deleted by some house keeping routines. To combat this, we monitor for this situation occurring, and stop the existing daemon and start a new one.

You can reproduce this easily in EmbeddedDBusDaemonTest by setting a breakpoint after the daemon.close(), you'll see that the daemonThread never exits. This also occurs if you use startInBackground().

This appears to be happening because the transport that is created in EmbeddedDBusDaemon.startListening() never gets closed. DBusDaemon.close() looks like it is supposed to do this, but transport.isConnected() at that point is returning false so the listening transport will not get closed. This is no chance for external code to access the transport and close it either.

In a way, this is related to #213, being in the same bit of code, the accept loop. Perhaps this is best solved properly in 5.0, but can you think of any work around to this?

@hypfvieh
Copy link
Owner

We have two issues here.
The first is: checking if transport.isConnected() is always false and it is also unnecessary, I can always call close() as long as transport is not null.

The second issue: The thread blocks in a BlockingQueue poll() operation. This blocks indefinite and will only stop if a message is available or if the thread gets an interrupt signal.
When close() is called on DBusDaemon object, the queue poll() operation is not terminated/interrupted - therefore the thread keeps running.
I fixed that as well and using join() in EmbeddedDBusDaemon.close() to ensure thread is terminated when close() returns.

@brett-smith
Copy link
Contributor Author

brett-smith commented Jun 29, 2023

Thanks as always. That has fixed the problem in my test environment. Feel free to close this.

As we needed this fixed quickly, I have published builds of 4.3.1-SNAPSHOT again to our company maven repository (https://artifactory.jadaptive.com/ext-snapshots-local if anyone is interested).

It made me think, have you considered publishing SNAPSHOT builds yourself? Being as you are already publishing to central yourself, its very low effort to get them on the "official" snapshot repository at https://oss.sonatype.org/content/repositories/snapshots.

You just add the following and mvn publish as normal. The rules for snapshots are more relaxed too, like i don't think they need to be signed.

<distributionManagement>
    <snapshotRepository>
	<id>ossrh-sshtools</id>
	<url>https://oss.sonatype.org/content/repositories/snapshots</url>
    </snapshotRepository>
    ..
    ..
</distributionManagement>

Edit: Ignore all of that, I see you already have that in the POM. I really should have checked :). I assume then 4.3.1-SNAPSHOT was just never published as a SNAPSHOT

@hypfvieh
Copy link
Owner

Maybe you can provide a proper PR for that?
If snapshots don't need to be signed I can change the github actions to publish new snapshots when I update master branch...

@brett-smith
Copy link
Contributor Author

brett-smith commented Jun 29, 2023

oh wow heh. replied at same time.

right ok... so the pom does already have it, then i guess you just need to do the Github thing.

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

2 participants