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

AsyncFoldLeft not threadsafe if collection is concurrently modified can result in closed Websockets #899

Open
squiddle opened this issue Feb 13, 2020 · 1 comment

Comments

@squiddle
Copy link

@squiddle squiddle commented Feb 13, 2020

Even though the listeners in ServerChannelImpl._listeners are a concurrently-modification safe CopyOnWriteArrayList the looping in BayeuxServerImpl#notifyListeners using AsyncFoldLeft is not.

AsyncFoldLeft#run takes the size of the collection at the beginning and calls List#get with an increasing index.
If the _listeners collection is changed during this left fold this will fail in either of 3 ways.

A if listeners got added, they will not be notified in this run
B if listeners got removed and added the new listeners are called
C if listeners got removed and the size is less than at the beginning an ArrayOutOfBoundsException will be thrown. This will then result in the websocket to be closed in line 72 of WebSocketEndPoint

C is a disaster
If A is correct behavior then B is wrong (if added listeners are not to be called it should not depend on coincidence of listeners being removed or not at the same time)
vice versa: if B is correct than A is wrong

i think the simplest correct fix would be to clone the listeners list before entering the AsyncFoldLeft to ensure it is a stable collection iterated over.
this would call removed listeners if they got removed after the loop started

more elegantly the indexedbased iteration could be replaced with using the iterator of the copyonwritearraylist which is also guarantueed to be stable even under concurrent modification.

cometd version: Bayeux 5.0.0-BETA1
jetty version: 9.4.25.v20191220

@squiddle

This comment has been minimized.

Copy link
Author

@squiddle squiddle commented Feb 14, 2020

I digged further through the code and the fix could be very minimal by removing the AsyncFoldLeft#run(List, Operation, Promise) specialized overload.

If that overload would be removed all current callsites would invoke AsyncFoldLeft#run(Collection, Operation, Promise) which uses Iterator based iteration that is working correctly for CopyOnWriteArrayList even under concurrent modifications (basically the iterator cannot see any modifications after it was created).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.