-
Notifications
You must be signed in to change notification settings - Fork 10
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
Rework XStream initialization #352
Conversation
39db9e5
to
c9bb976
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the problem is concurrent invocation of registerXStream
, wouldn't it be more sensible to make GamePlugin
a singleton?
That function is already static. That doesn't solve the concurrency problem, unfortunately. |
I have run all tests repeatedly locally with no failure. The one failing test has been unreliable for a while, I'll see whether I have to Disable it or find a fix. |
As far as I can tell, that function is only called on initialisation of GamePlugin, right? |
you're right, the concurrency issues occur only in the tests. Hmm... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are succeeding on my machine with the current state 👍
I think the actual issue is still why that function is being called several types at all |
As said, parallelized tests |
yes, but this should be setup stage for tests. This shouldn't be something several tests initialise for themselves anyways. |
yeah, the question is whether there is a way to have global setup for independent tests. |
ecafced
to
8d3b70b
Compare
hello it's me, giant commit 😄 yes I just changed a lot of code. I will also add some documentation for that in the GUIDELINES soon. But unlike last time, where I made the XStream instance global (brevity at the expense of creating global state, which is the opposite of clean code ^^), this really does make the code a whole lot cleaner (which we can partly see through the application of the design patterns Factory + Template Method)! |
8d3b70b
to
1832b38
Compare
Due to the tests now running in parallel, we sometimes had ConcurrentModificationExceptions since the global XStream instance was initialized in parallel from different threads. Now all initialization points are locked.
…tener Thus we save the synchronization overhead.
1832b38
to
c2365c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Due to the tests now running in parallel, we sometimes had ConcurrentModificationExceptions since the global XStream instance was initialized in parallel from different threads.
Now all initialization points are locked.