Skip to content

fix: change the event type that SqlSessionFactory is listening to #797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

SkyAndCode
Copy link
Contributor

fixes #796

This is why I need to change it.

  1. From the implementation of SqlSessionFactory, I think that it does not need to listen to all ApplicationEvents, and only needs to listen to ContextRefreshedEvent.
  2. When there are multiple ApplicationContexts in Spring, ContextClosedEvent may be broadcasted multiple times. For example, FeignContext, since SqlSessionFactory listens to all ApplicationEvents, if the application is closed, when SqlSessionFactoryBean is destroyed first, and FeignContext is destroyed later, ContextClosedEvent will be broadcasted. When Spring tries to get the listener, BeanCreationNotAllowedException is thrown because SqlSessionFactoryBean's singleton bean has been destroyed and it indicates that singletons are currently being destroyed (singletonsCurrentlyInDestruction=true).
    Although Spring considers this only a warning, I think that SqlSessionFactoryBean does not need to listen to all ApplicationEvents, and this warning should not appear in this case.

@SkyAndCode SkyAndCode force-pushed the change-sqlsessionfactory-event-type branch from 518dc8c to e8a2a57 Compare March 30, 2023 12:55
@hazendaz hazendaz requested a review from kazuki43zoo April 7, 2023 00:14
@hazendaz
Copy link
Member

hazendaz commented Apr 7, 2023

@kazuki43zoo This seems logical given the override only checks ContextRefreshedEvent before doing anything, your thoughts?

@coveralls
Copy link

Coverage Status

Coverage: 89.392%. Remained the same when pulling e8a2a57 on SkyAndCode:change-sqlsessionfactory-event-type into 531bd26 on mybatis:master.

@hazendaz hazendaz self-assigned this Apr 23, 2023
@hazendaz
Copy link
Member

no feedback, I'm pretty sure this is good, merging.

@hazendaz hazendaz merged commit e5d967d into mybatis:master Apr 23, 2023
@kazuki43zoo kazuki43zoo added the polishing Improve a implementation code or doc without change in current behavior/content label May 13, 2023
@kazuki43zoo kazuki43zoo added this to the 3.0.2 milestone May 13, 2023
@kazuki43zoo kazuki43zoo added enhancement Improve a feature or add a new feature backport to other version Backport to other maintenance version and removed polishing Improve a implementation code or doc without change in current behavior/content labels May 13, 2023
@kazuki43zoo kazuki43zoo added the done backported Backporting is doned label May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport to other version Backport to other maintenance version done backported Backporting is doned enhancement Improve a feature or add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When Spring has multiple ApplicationContexts, it may throw BeanCreationNotAllowedException
4 participants