core: Avoid locks in SynchronizationContext

[njhill]

Similar to what's done in SerializingExecutor, it's easy to make SynchronizationContext non-blocking.

[carl-mastrangelo]

LGTM

@zhangkun83 PTAL

I know this isn't super performance critical, but this has the nice benefit that the app thread can no longer block the network thread.

[dapengzhang0]

This may change the current behavior because the ordering of the runnables may be changed. There seems no harm for the ordering change (May need confirm from @zhangkun83 ). This change seems better, because executeLater becomes always not blocking after the change.

[dapengzhang0]

Discussed offline, the behavior is more or less the same. So no concern here.

[zhangkun83]

There seems to be a race:

T1: call drain(): set drainingThread. Finish all tasks, exit loop
T2: call executeLater(): add a new task
T2: call drain(): try to set drainingThread, but is still set, give up
T1: still in drain(): reset drainingThread, then exit

End result: the task added by T2 will not be run unless someone calls drain() again.

[zhangkun83]

Nm, I see you have another loop to catch that.

[dapengzhang0]

T1: still in drain(): reset drainingThread, then exit

For T1, because there is while (!queue.isEmpty()) in the end, it will drain again, not exit.

[dapengzhang0]

I agree without lock it's harder to prove the correctness and easier to break the correctness in the future though. But this seems to have the same complexity as the SerializingExecutor.

[njhill]

Thanks @carl-mastrangelo @dapengzhang0 @zhangkun83. A possible variation would be to use an AtomicBoolean and ThreadLocal<Boolean> instead of the AtomicReference<Thread>, i.e. no need to use Thread.currentThread(). This would make throwIfNotInThisSynchronizationContext cheaper - just a TL instead of volatile read. But as @carl-mastrangelo said, not on request path so I guess not that perf-critical.

[zhangkun83]

LGTM