Prevent concurrency issues during XStream SPI lookup - #5731
Conversation
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
|
It might be better explained by that it tries to download the XSD during the build but failed: |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
I do not think this change addresses the linked build failure.
The failure occurred in maven-jaxb2-plugin while XJC was resolving the imported config-description-1.0.0.xsd, and the underlying exception was a connection timeout. The subsequent undefined config-description:* types are a consequence of that imported schema not being loaded.
XmlDocumentReader and XStream are not part of that execution path, so the successful retry does not provide evidence of an XStream SPI race. Before adding a JVM-wide synchronization mechanism, could you provide a separate reproducible failure or stack trace that actually involves concurrent XStream initialization?
| * </ol> | ||
| */ | ||
| public XmlDocumentReader() { | ||
| synchronized (XSTREAM_SPI_LOCK) { |
There was a problem hiding this comment.
This block does not appear to serialize the StAX SPI lookup described above.
In XStream 1.4.21, constructing StaxDriver only initializes its fields. XMLInputFactory.newInstance() and XMLOutputFactory.newInstance() are invoked lazily when the driver first reads or writes XML, after this synchronized block has already been exited.
Therefore, even assuming concurrent StAX provider lookup is the problem, concurrent first use of different readers would remain unprotected. Could this first be demonstrated with a reproducer or concurrency test and then fixed around the operation that actually fails?
There was a problem hiding this comment.
If that's the case, this is all pointless. How are we supposed to handle the thread-unsafe SPI mechanism then? Perhaps it would be possible to do something with the newly created instance that forced instantiation within the lock?
| * SPI lookups using this lock, there should be no opportunity for concurrency issues. The lock object is public | ||
| * to enable other classes to apply the same lock. The lock object itself shares its lifecycle with the JVM. | ||
| */ | ||
| public static final Object XSTREAM_SPI_LOCK = new Object(); |
There was a problem hiding this comment.
Could we avoid exposing a raw monitor as public API?
Any consumer could hold this lock or combine it with other locks, potentially stalling XML reader construction or introducing lock-ordering problems. It also relies on every XStream user voluntarily synchronizing on the same object.
If a shared workaround is still needed after reproducing the issue, a narrow factory or helper API with a private lock would provide better encapsulation.
That might very well be the actual reason, however I found nothing that indicated that the XSD wasn't available at the time, which is why I looked for other explanations.
As I said initially, I do not know what causes this. It's a guessing game, I assume that the timeout is long enough that a very brief network issues shouldn't prevent it from succeeding. This was two weeks ago now, so I don't even remember the reasoning I made at the time. There is no doubt that the undefined types are a result of the schema not being loaded, the question is why the loading fails. I'm pretty sure that I've seen this build failure before, but it's quite infrequent in my experience. What are the chances that these are actually intermittent network failures? I don't know, but I think it's a bit strange, I would think that if the website was down or unreachable, it wouldn't be just for a few seconds while this times out. There are often other, parallel builds happening at the same time, that don't fail, in this case the Java 25 build took place pretty much "in sync" with this one, yet only the Java 21 one failed. If it was a network issue, I'd think that both would fail.
No, I have no idea how to reproduce it. I've never had this locally, I've only seen it with the GitHub runners, and have no idea where to even start to try to reproduce it. My idea is that since the SPI resolution is known not to be thread-safe, and we know that it's involved at some stage in this process, taking care of that might solve the problem. It certainly couldn't hurt. |
|
I think labelling this When you demand proof that it was the cause, and label it I was trying to be helpful in getting rid of some of the flaky tests that plague the project. Finding the cause is no more my responsibility than anybody else's, and if you prefer to have flaky tests over something that might solve it, and potentially other issues (even though I assume that this particular SPI resolution don't happen a lot, probably only during startup), that's your choice. I'm not going to try to solve the details when the premise is dismissed. |
|
The "invalid" label was not intended as a judgment on your effort. It refers to the premise of this PR. The failure points to an XSD download timeout in XJC, while this change modifies XStream initialization on a different execution path. Asking for a reproducer, relevant stack trace, or test is reasonable before introducing a public JVM-wide lock, especially since the synchronized block does not appear to cover the suspected StAX lookup. I appreciate that you were trying to improve build reliability, but please keep the discussion technical and avoid suggesting that reviewers prefer flaky tests. |
That is part of the "technical" argument in this case. It was no secret that this was an attempt at fixing what I thought was a likely cause at the time, although I don't remember the exact reasoning anymore. Especially since the fix was likely to have no negative effects, since as I understood it, this SPI resolution only takes place once during startup at runtime, but repeatedly during testing. The fact remains that the SPI resolution is still done without concern for concurrency. When you prefer to let a known bug exist because it might not be the cause of this particular failure, what conclusion am I to make? You might have had a good point regarding the lazy initialization, and I find it hard to get down to the exact details of the SPI resolution process, debugging it wasn't successful, and I don't know what kind of "magic" the JVM does. Following that trail would have been productive as I see it, but calling it invalid and talking about proving that this was the exact failure mode here, is hard to take seriously. You might as well just say "Unless you can prove the Riemann Hypothesis, I'm not interested". I'm not suggesting that you prefer flaky tests in general, but that you care more about "preserving the status quo" than trying to improve things. And when the "status quo" is flaky tests... It's not the first time I've seen this. It happens with some runners while others pass at the same time. Why are there certain tests that fail repeatedly while most others don't, if the cause is a network issue? Why do the other builds run in parallel not fail? These are "clues" that tell me that this is most likely a concurrency issue, not a network issue. But, of course, I might just have been extremely unlucky when this exact test runs. |
Please check the facts and my contribution history before making claims like this: |
I wasn't making a general claim, it was about this specific situation. I thought (and still think) that the symptoms matched that a concurrency issue, not a network failure. That was the reason I went that way in the first place. As far as I remember, I thought I found some explanation for how the concurrency issue could lead to the timeout elapsing. When looking at the stack trace now, I don't quite see where, because it seems like all the xerces stuff happens "above" the network code that probably throws the exception. But that was the idea that led down this path. When looking at the SPI loading again now, I don't think that a lock there would solve anything. It appears that what I read about it being thread-unsafe was somewhat mislead. I haven't managed to find the exact details, but there is a cache of the providers, that is essential in all of this. What I understood previously was that this cache was static/shared, but when reading the source code of #5757 makes sense anyway, because it's good to decouple building from the state of the web page. I don't know if it will resolve this particular issue because I don't think the observed behavior matches that of a network failure. If it does, then the GitHub runners have a "strange" network issue. But in either case, it seems like this PR was a dead end. |
2 participants
I just had a CI build failure in #5720 that makes no sense to me: https://github.com/openhab/openhab-core/actions/runs/30165880084/job/89698693351?pr=5720
It seems unrelated to anything I've done in the PR, so I suspected a flaky test. Force pushing the branch with no actual changed (just a new hash for the last commit) triggered a new build that didn't fail, which I interpret as a confirmation that it was a flaky test.
I've tried to dig in and find what could possibly have caused it, and I can't be 100% sure, but SPI lookups aren't thread-safe. Each time a
XmlDocumentReaderinstance is created, an SPI lookup is done to find the actual XML parser implementation. My theory is that while tests are being run concurrently, multiple SPI lookups might happen to happen at the same time, which makes them fail/see incomplete data, which could explain the above error.I can't guarantee that this is what's causing the flakiness, but it seems plausible, and knowing that SPI lookups aren't thread-safe, it can hardly hurt for force serialization of the lookups.
There are add-ons that also create
XStreaminstances, but they are very unlikely to occur in parallel with anything else, so I think it's acceptable to let them remain "unprotected". I've made the lock public though, in case there is a situation where there's a need for other parts of the code to share the same lock.