Add guards around structures to prevent racing. #74
Conversation
| static List<Category> categories = new ArrayList<Category>(); | ||
|
|
||
| static List<Pet> pets = Collections.synchronizedList(new ArrayList<Pet>()); | ||
| static List<Category> categories = Collections.synchronizedList(new ArrayList<Category>()); |
There was a problem hiding this comment.
I guess you don't need to synchronize this, as it is not used anywhere outside the static initializer.
|
|
||
| public class StoreData { | ||
| static List<Order> orders = new ArrayList<Order>(); | ||
| static List<Order> orders = Collections.synchronizedList(new ArrayList<Order>()); |
There was a problem hiding this comment.
If you always access this in a synchronized block, there is no need for the additional synchronizedList.
| @@ -68,11 +69,13 @@ public Pet getPetById(long petId) { | |||
|
|
|||
There was a problem hiding this comment.
So you in almost all methods synchronize on pets, just not in getPetById (some lines up from here). Intentionally?
I'm not sure if this here could create some issue ...
The iterator is not synchronized, but checks for concurrent modifications (in an unsynchronized way).
So if any other request gets in between with deletion/addition, this could throw an exception.
On the other hand, if you synchronize everything anyways, you could get rid of the synchronizedList.
There was a problem hiding this comment.
I missed the iterator around it. I also removed the synchronizedLists since all the operations are within sync {}.
|
@antihax thanks for the PR. Can you submit it against my repo instead: https://github.com/wing328/swagger-samples ? That way we can more easily test and deploy. |
|
updated based on review. I will see if i can figure out to submit to your repo. |
@wing328 From what I can tell, there are races around the data structures here when hit concurrently.
Please review and make sure my use of
Collections.synchronizedListandsynchronizedare correct. I have not written anything in Java for some time.I have these changes running locally with
go teston a loop and beefed up the concurrency significantly in the go tests. Running 30 minutes with no failures now. This should resolve swagger-codegen #5102.