First pass - trivial synchronized blocks

[tzolov]

• Convert the "trivial" synchronized block into reentrant locks.

Resolves #8643

[artembilan]

Mostly this is OK, but really too much and it is easy to miss something.
Please, see if you really were right about changing tryLock() to lock() everywhere you found it.

@garyrussell ,

as we already know this is easier to review with ?w=1.

@tzolov ,

let's see next time if can propose the change in baby steps, per use-case. Really hard to review.

[artembilan]

May we have this optimized like call this.lock.lock() only once?
What I mean we probably need to extract a private Graph doBuildGraph() without lock and have it called from this buildGraph() surrounded with lock and also call it from that getGraph().
Right now we from that getGraph() we have two this.lock.lock().
Although it might be just my paranoia and that is not a big deal since we would have just increased count on the lock.

And that it, you know, why huge PRs are not OK, when they touch a lot of not related stuff 🤷

[tzolov]

Technically it should work as it is given the preemtive nature of the locks.
But looking at the getGraph() it seems like the mutex is redundat there. I belive something like this should be correct:

	public Graph getGraph() {
		if (this.graph == null) {
			buildGraph(); // already under mutex
		}
		return this.graph;
	}

what do you think?

[artembilan]

If several threads call this getGraph() and encounter with this.graph == null, they step into that buildGraph() and wait for a lock.
When first one is done with building, it releases lock and the next one takes it and start building a fresh graph.
We do that unconditionally in the rebuild(), but getGraph() should build only once.

Well, let's leave it as you have in the PR right no and revise it later in the separate change!

[artembilan]

Just one concern so far I've noticed.

[artembilan]

Looks like the change in STOMP module has affected somehow tests:

StompInboundChannelAdapterWebSocketIntegrationTests > testWebSocketStompClient() FAILED
    java.lang.AssertionError at StompInboundChannelAdapterWebSocketIntegrationTests.java:162

[tzolov]

StompInboundChannelAdapterWebSocketIntegrationTests > testWebSocketStompClient() FAILED
java.lang.AssertionError at StompInboundChannelAdapterWebSocketIntegrationTests.java:162

It is a bit strange as we have not changed much on the Stomp Inbound channel code.
Anyway the core of the issue is even stranger as it complains that

Error creating bean with name 'webSocketEventListener' defined in org.springframework.integration.stomp.inbound.StompInboundChannelAdapterWebSocketIntegrationTests$ServerConfig: 
Unsatisfied dependency expressed through method 'webSocketEventListener' parameter 0: 
No qualifying bean of type 'org.springframework.messaging.support.AbstractSubscribableChannel' available: 
expected single matching bean but found 3: clientInboundChannel,clientOutboundChannel,brokerChannel

After qualifying explicitly the taskScheduler :

@Bean
public WebSocketStompClient stompClient(@Qualifier("taskScheduler") TaskScheduler taskScheduler) {
...

and the clientOutboundChannel :

@Bean
public ApplicationListener<SessionSubscribeEvent> webSocketEventListener(@Qualifier("clientOutboundChannel") final AbstractSubscribableChannel clientOutboundChannel) {
...

The test passes. But i'm no sure if this fixes or masks the core reason that lead to this.

[artembilan]

This is OK with me.

@garyrussell ,
let us know if you are going to review it, or just let it go and come back to specific concern when it raises!

[garyrussell]

I don't see a need for me to review.

Thanks.

[artembilan]

Then I'll merge this as is and we can revise other concerns in other PRs or so.