Transacrion/Span#finish should be thread-safe

[marandaneto]

The method finish should be thread-safe as it could be called from multiple threads.

cc @Tyrrrz @brustolin

[bruno-garcia]

Finish is expected to be called once though

[marandaneto]

Finish is expected to be called once though

same for SDK init., still, as discussed, it'd be ideally thread-safe, this gets more important if we decide to create the transaction when finish is called in the root Span.

[bruno-garcia]

Transaction doesn't need to be thread-safe.

Sentry.init is a static method. An atomic ref swap on init is a cheap enough way to make sure we don't end up with odd behavior if init is called while the SDK is already being used by some other thread. That doesn't mean we need to make all types thread-safe.

SentryEvent isnt' thread-safe and that's OK. Transaction is like SentryEvent.
Making finish "thread-safe" will have lots of side effects. Either we'll make the whole type thread-safe, or not bother at all with a one off method.

I'd be more worried about something like setTag if the transaction object is being shared by multiple-threads (say, bound to the scope). As we set tags in parallels would definitely crash the process.

We need to be clear on what is designed to be accessed concurrently, and what is not. And for an instance type, unless explicitly documented to be thread-safe, assume not to be. For static types it's fair to assume something is thread-safe though, ideally side-effect free even.

Ultimately we could have an atomic bool that flips once the Span closes so that we avoid "closing it twice". In the case of Transaction that has the side effect of flushing itself it would have the benefit of not capturing the same transction twice. Even if called sequentially, by the same thread:

transaction.finish();
transaction.finish();

[marandaneto]

@bruno-garcia while I don't disagree with your whole explanation, I don't understand what/how made you change your mind from our last call, we've discussed that and we agreed that making finish thread-safe would make a lot of sense if we go ahead with refactoring things like #1251

also, a transaction would be only created when the root span was finished, etc...
so either I lost a lot of Context or we were talking about different things.

[marandaneto]

an example would be this: getsentry/sentry-cocoa#950 (comment)

if we go ahead with something like that, IMO finish must be thread-safe, as spans, tags, etc... could be mutated in different threads