javamelody / grails-melody-plugin

JavaMelody monitoring plugin for Grails, to monitor application performance
https://plugins.grails.org/plugin/grails-melody-plugin
Apache License 2.0
32 stars 31 forks source link

Exception: Connection has already been closed #53

Closed Relaximus closed 6 years ago

Relaximus commented 6 years ago

With update to the 1.70.0 version of Melody. We avoided the #48 but it seems that we have got the jdbc connections leakage.

From our log:

Caused by SQLException: Connection has already been closed. ->> 197 | invoke in net.bull.javamelody.JdbcWrapper$ConnectionInvocationHandler

We suppose, that we catch the bug here, because we are using pooled option in our Grails app datasource configuration.

Comparing the log output of 1.67 and 1.70 we noticed that the 1.70 wraps one additional bean dataSourceUnproxied. Excluding this bean from wrapping seems helped as a workaround.

Could you check the BBP approach in conjunction with db pool enabled?

sergiomichels commented 6 years ago

@bassmartin Could you check it?

bassmartin commented 6 years ago

@sergiomichels see #52 I think adding 'excludes' like I wrote in my ticket will fix this issue too.

Relaximus commented 6 years ago

@bassmartin That is interesting, because our workaround is other way around to exclude unproxied bean and it works. It seems that the problem, when Melody wrapper wraps itself twice. What do you think?

bassmartin commented 6 years ago

I think it really has to do with what you say "re-wrapping".

The test I was doing is to run my app, close my wi-fi (so DB was not accessible anymore), wait for tons of error to show, reconnect the wifi, expect the background jobs to work a expected and the UI to show queried data without any error.

Only proxying the 'unproxied' data sources would pass this test.

But this morning, in an integration environment, I seem to see the same issue happening again.

I will try your solution this PM.

bassmartin commented 6 years ago

I finished doing the same test with 'dataSource' being the only one not excluded from being proxied and it's causing "org.postgresql.util.PSQLException: This connection has been closed." issues.

bassmartin commented 6 years ago

I just tried only proxying 'dataSourceLazy' and it seems to pass my test. I've done it twice. I will continue to play with this.

resources.groovy // Recreate the bean with excluded datasources springDataSourceBeanPostProcessor(SpringDataSourceBeanPostProcessor) { excludedDatasources = ['dataSourceUnproxied', 'dataSource'] }

bassmartin commented 6 years ago

The fix is still in our integration environment. So far so good.

sergiomichels commented 6 years ago

@bassmartin This fix is working for you, right? Could you open an PR?

sergiomichels commented 6 years ago

I've just created a sample app with latest Grails version and cannot see any sql statistics :(...

EDIT - Looks like we cannot use SpringDataSourceBeanPostProcessor anymore. Grails uses now InstanceFactoryBean wrapping the DataSource proxies

bassmartin commented 6 years ago

The fix is working fine in my env. (Grails 3.2.x). I will create a PR if you still want it.

So we will need to create a new release line for Grails 3.3.x?

sergiomichels commented 6 years ago

I think we can maintain one version but if it's Grails 3.3.x we will need to load a different BeanPostProcessor. No need for PR, I will test it for both versions :)

bassmartin commented 6 years ago

I recently upgraded to GORM 6.1+ and the fact that

EDIT - Looks like we cannot use SpringDataSourceBeanPostProcessor anymore. Grails uses now InstanceFactoryBean wrapping the DataSource proxies

is caused by GORM 6.1+, not Grails 3.3 per se (I'm still on Grails 3.2+)

bassmartin commented 6 years ago

Any progress on this issue? Or do you have some pointers so I can try to implement something to get SQL monitored again? Thanks.

bassmartin commented 6 years ago

I've put back the code in GrailsMelodyPluginGrailsPlugin that was there before switching back to SpringDataSourceBeanPostProcessor. I changed the if clause so it only wraps the Lazy datasource beans. SQL queries a monitored. I will check if after a while there are issues arising. (Grails 3.2.13 and Gorm 6.1.9).

void doWithApplicationContext() { //Need to wrap the datasources here, because BeanPostProcessor didn't worked. def beans = getApplicationContext().getBeansOfType(DataSource) beans.each { beanName, bean -> if (beanName.contains('Lazy')) { LOG.debug "Wrapping DataSource - $beanName" bean.targetDataSource = JdbcWrapper.SINGLETON.createDataSourceProxy(bean.targetDataSource) } } }

see https://github.com/javamelody/grails-melody-plugin/compare/master...bassmartin:grails_3.2_gorm_6_1_datasource_proxy

bassmartin commented 6 years ago

Still no issue and queries are logged.

sergiomichels commented 6 years ago

Thanks @bassmartin, could you open an PR?

bassmartin commented 6 years ago

I will today.

bassmartin commented 6 years ago

See #61

sergiomichels commented 6 years ago

@bassmartin I merged, but still cannot see sql info in a simple project (tested installing in maven local as snapshot)... Are you using JNDI?

bassmartin commented 6 years ago

No, I'm no using JNDI. I'm using Grails 3.2.13 and Gorm 6.1.9 Can you test your simple project with that?

bassmartin commented 6 years ago

Might have to do with

Datasource Plugin Refactor In previous versions of Grails and GORM the multiple data sources support relied on Grails' data sources plugin. The logic for configuring multiple data sources has moved to GORM and as a result of major changes to the dataSources plugin, beans for the lazy and unproxied representation of a dataSource are no longer available.

The beans include:

dataSourceUnproxied

dataSourceLazy

If you are referencing these beans you will need to remove these references and unwrap the single dataSource proxy manually.

We might need to add a check on the Grails version and proxy either 'dataSourceLazy' in Grails <= 3.2 or 'dataSource' in Grails >= 3.3 ?!?!

sergiomichels commented 6 years ago

You're right, Grails 3.2.x works and 3.3 fails, we'll have to conditionally wrap data sources indeed.

bassmartin commented 6 years ago

I created #62 that first attempt 'Lazy' beans then fallbacks on 'dataSource' if no 'Lazy' found. So it should support Grails <=3.2 as well as Grails >= 3.3

sergiomichels commented 6 years ago

Thanks @bassmartin, I've tested here and is logging sql info now.