You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2021/05/15 03:44:42 UTC
[GitHub] [zeppelin] xiejiajun opened a new pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
xiejiajun opened a new pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119
### What is this PR for?
* Fix the problem of Ldap connection leak in LdapGroupRealm
### What type of PR is it?
[Bug Fix]
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5372
### How should this be tested?
* Ci pass
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] cuspymd commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
cuspymd commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-841980605
> The close method of LdapCtx will call the close method of LdapClient to return the connection to the connection pool. If we rely on the GC mechanism, although the Context object can be cleared, the connection will not be returned normally. And a new connection will be created the next time we try to obtain a connection, which will cause connection leakage.
As bellow code of `LdapCtx::finalize()`, `LdapCtx::close()` called also when it be released by GC.
```
protected void finalize() {
try {
this.close();
} catch (NamingException var2) {
}
}
```
In both cases, the same function(`LdapCtx::close()`) is called, but I don't understand that connection leaks occur only when released by the GC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] xiejiajun closed pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
xiejiajun closed pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] xiejiajun commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
xiejiajun commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-841669228
> Thanks for the clarification. PR LGTM.
> BTW, do you know whether there's some connection pool library that we can use to manage the lifecycle of LDAP connection ? Let user to manage the connection lifecycle doesn't seem a proper solution.
I think even if we can manage the LDAP connection through a third-party connection pool library, we need to manually return the connection to the pool after using up the connection, which is the same as the current solution, or do you have any more detailed suggestions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-841646018
Thanks for the clarification. PR LGTM.
BTW, do you know whether there's some connection pool library that we can use to manage the lifecycle of LDAP connection ? Let user to manage the connection lifecycle doesn't seem a proper solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] xiejiajun commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
xiejiajun commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-841641694
> Not sure whether this is the root cause. According the javadoc of Context#close, it looks like even close is not called, it would be released.
>
> ```
> /**
> * Closes this context.
> * This method releases this context's resources immediately, instead of
> * waiting for them to be released automatically by the garbage collector.
> *
> * <p> This method is idempotent: invoking it on a context that has
> * already been closed has no effect. Invoking any other method
> * on a closed context is not allowed, and results in undefined behaviour.
> *
> * @throws NamingException if a naming exception is encountered
> */
> public void close() throws NamingException;
> ```
@zjffdu
The close method of LdapCtx will call the close method of LdapClient to return the connection to the connection pool. If we rely on the GC mechanism, although the Context object can be cleared, the connection will not be returned normally. And a new connection will be created the next time we try to obtain a connection, which will cause connection leakage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] xiejiajun commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
xiejiajun commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-842091804
> > The close method of LdapCtx will call the close method of LdapClient to return the connection to the connection pool. If we rely on the GC mechanism, although the Context object can be cleared, the connection will not be returned normally. And a new connection will be created the next time we try to obtain a connection, which will cause connection leakage.
>
> As bellow code of `LdapCtx::finalize()`, `LdapCtx::close()` called also when it be released by GC.
>
> ```
> protected void finalize() {
> try {
> this.close();
> } catch (NamingException var2) {
> }
>
> }
> ```
>
> In both cases, the same function(`LdapCtx::close()`) is called, but I don't understand that connection leaks occur only when released by the GC.
@cuspymd Thx, I re-tracked the source code according to your prompts. This is indeed not the root cause of the connection leak. I will close this PR and investigate other reasons for the connection leak.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] cuspymd commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
cuspymd commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-842098452
@xiejiajun Thanks for your checking too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] xiejiajun commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
xiejiajun commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-842091804
> > The close method of LdapCtx will call the close method of LdapClient to return the connection to the connection pool. If we rely on the GC mechanism, although the Context object can be cleared, the connection will not be returned normally. And a new connection will be created the next time we try to obtain a connection, which will cause connection leakage.
>
> As bellow code of `LdapCtx::finalize()`, `LdapCtx::close()` called also when it be released by GC.
>
> ```
> protected void finalize() {
> try {
> this.close();
> } catch (NamingException var2) {
> }
>
> }
> ```
>
> In both cases, the same function(`LdapCtx::close()`) is called, but I don't understand that connection leaks occur only when released by the GC.
@cuspymd Thx, I re-tracked the source code according to your prompts. This is indeed not the root cause of the connection leak. I will close this PR and investigate other reasons for the connection leak.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] xiejiajun closed pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
xiejiajun closed pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] cuspymd commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
cuspymd commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-841980605
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4119: [ZEPPELIN-5372]. Fix the problem of Ldap connection leak in LdapGroupRealm
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4119:
URL: https://github.com/apache/zeppelin/pull/4119#issuecomment-841598895
Not sure whether this is the root cause. According the javadoc of Context#close, it looks like even close is not called, it would be released.
```
/**
* Closes this context.
* This method releases this context's resources immediately, instead of
* waiting for them to be released automatically by the garbage collector.
*
* <p> This method is idempotent: invoking it on a context that has
* already been closed has no effect. Invoking any other method
* on a closed context is not allowed, and results in undefined behaviour.
*
* @throws NamingException if a naming exception is encountered
*/
public void close() throws NamingException;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org