Fix unsynchronized access on subscription cancellation

[vitaut]

We shouldn't acquire lock in the range expression for reasons explained in https://fburl.com/yl0onxkm.

[vitaut]

There is more to it than meets the eye. Even if you don't need synchronization for elements, you can still have a crash when accessing an iterator that was invalidated by racing push_back in cancel. Therefore you need to hold the lock for the whole duration of iteration.

This won't fix all the synchronization issues

Sure, the purpose of this diff is to fix the most severe and obvious misuse of Synchronized that may result in a crash, not fix all the synchronization issues in the class. Please open an issue for latter.

[dymk]

Still needs an exclusive lock (eg wlock(), or consider folly::Synchronized<..., std::mutex>)

[vitaut]

@dymk, could you elaborate why you think an exclusive lock is needed when iterating over tiedSubscriptions_?

[dymk]

@vitaut Because we're mutating the elements of tiedSubscriptions_, by canceling them. We also don't want to call cancel in a non-synchronized way, according to the reactive streams spec.

[vitaut]

Changed to write locks.