Skip to content

Conversation

@jonenst
Copy link
Contributor

@jonenst jonenst commented Sep 12, 2025

The @transactional was already unneeded, it didn't propagate to the executor thread Effectly it opened a transaction and instantly closed it.

Spring forces us to adapt the tests with boilerplate code (asyncStarted() and asyncDispatch()) as per https://docs.spring.io/spring-framework/reference/testing/mockmvc/hamcrest/async-requests.html to mimic the servlet 3.0 redispatch

….0+ AsyncContext)

The @transactional was already unneeded, it didn't propagate to the executor thread
Effectly it opened a transaction and instantly closed it.

Spring forces us to adapt the tests with boilerplate code (asyncStarted() and asyncDispatch()) as per
https://docs.spring.io/spring-framework/reference/testing/mockmvc/hamcrest/async-requests.html
to mimic the servlet 3.0 redispatch
};
}

@Transactional(readOnly = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we move this Transactional somewhere else ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest in a separate PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also in this case it looks like all the computation is just a single repository call to the standard find method so it should be equivalent I think.. not 100% sure from the top of my head though. Also not sure what is the official code to do transactions in a separate thread.. probably it's just the same but I would need to double check..)

} else {
return getSubstationsByCountries(network, countrySet);
} catch (Exception e) {
throw new GeoDataException(FAILED_SUBSTATIONS_LOADING, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it behave the same to throw inside or outside the async ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it's the same. And there are unit tests that verify that from an http client point of view, the thrown exception inside the completablefuture is correctly used by the servlet container asynchronously to return the associated error http reponse to the client

@jonenst jonenst requested a review from klesaulnier October 9, 2025 14:55
@sonarqubecloud
Copy link

@jonenst jonenst merged commit 2d70522 into main Oct 14, 2025
4 checks passed
@jonenst jonenst deleted the noblocktomcatthread branch October 14, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants