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

Fix daemon connection issues #1071

Merged
merged 1 commit into from
Jul 19, 2024
Merged

Fix daemon connection issues #1071

merged 1 commit into from
Jul 19, 2024

Conversation

oehme
Copy link
Contributor

@oehme oehme commented Jul 16, 2024

We have a test suite for a Maven extension that runs around 5000 builds and before this change, on each CI build, about 100 of them would fail with daemon connection issues. The symptom would always be the same - The daemon is waiting for the build request, the client has already sent it, but the daemon never receives it, no matter how long we configured the timeouts.

The reason turned out to be the way that the daemon waits for the request:

The Server was using a SynchrnousQueue to coordinate the main thread
and the background thread that receives the request from the client.
A SynchronousQueue only allows insertions when a corresponding call
to get is in progress. However, since the receiver thread is started
before the call to get, there was a short time window, where the call
to queue.offer could fail and simply return false. This return code
was ignored.

A possible solution would have been to call put instead of offer,
but I decided to replace the queue with a Future, since we only wait
for a single element.

@gnodet
Copy link
Contributor

gnodet commented Jul 17, 2024

It seems much less stable on Windows for some reason...

@oehme
Copy link
Contributor Author

oehme commented Jul 17, 2024

Yep, looks like I'll have to dig into the Windows behavior some more. Taking back into draft.

@oehme oehme marked this pull request as draft July 17, 2024 14:00
@oehme oehme force-pushed the mvnd-1.x branch 3 times, most recently from 360805c to 96baf69 Compare July 18, 2024 15:04
@oehme
Copy link
Contributor Author

oehme commented Jul 18, 2024

After a lot of debugging, I've finally figured out the true cause of this, see the updated description.

I guess my previous attempt had simply changed the timing slightly, reducing the flakiness on my machine, but increasing it in the Windows builds, which explains those failures.

@oehme oehme marked this pull request as ready for review July 18, 2024 15:18
@gnodet
Copy link
Contributor

gnodet commented Jul 18, 2024

After a lot of debugging, I've finally figured out the true cause of this, see the updated description.

I guess my previous attempt had simply changed the timing slightly, reducing the flakiness on my machine, but increasing it in the Windows builds, which explains those failures.

Really nice catch.

However, even if guava is a transitive dependencies, it's not really a direct dependency in maven.
I would really prefer if we could use a JDK collection somehow, maybe new ArrayBlockingQueue(1) ?

The Server was using a SynchrnousQueue to coordinate the main thread
and the background thread that receives the request from the client.
A SynchronousQueue only allows insertions when a corresponding call
to `get` is in progress. However, since the receiver thread is started
before the call to `get`, there was a short time window, where the call
to `queue.offer` could fail and simply return `false`. This return code
was ignored.

A possible solution would have been to call `put` instead of `offer`,
but I decided to replace the queue with a Future, since we only wait
for a single element.
@oehme
Copy link
Contributor Author

oehme commented Jul 18, 2024

Good point, changed to CompletableFuture

@gnodet gnodet added this to the 1.0.2 milestone Jul 18, 2024
@gnodet gnodet added bug Something isn't working backport backport to a maintenance branch labels Jul 19, 2024
@gnodet gnodet merged commit bb1a4e1 into apache:mvnd-1.x Jul 19, 2024
5 checks passed
@ppalaga
Copy link
Contributor

ppalaga commented Jul 28, 2024

Indeed, great catch @oehme 🙏

@gnodet gnodet mentioned this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport backport to a maintenance branch bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants