Conversation
butonic
force-pushed
the
reuse-ldap-connection
branch
from
8f6cf2d to
4ee2b75
Compare
longsleep
left a comment
longsleep
left a comment
There was a problem hiding this comment.
The LDAP backend is really minimal and I am glad to see some improvements. Added a bunch of comments for discussion.
| b.connLock.Lock() | ||
| defer b.connLock.Unlock() | ||
|
|
||
| if b.conn != nil && !b.conn.IsClosing() { |
There was a problem hiding this comment.
This does not really know if the connection actually still works and might result in error and thus logon failure when getting used. Does the ldap connection do anything (like in the background) to ensure that does not happen? Normally that would be the job of the pool before giving the connection to the caller.
There was a problem hiding this comment.
Hm, the connection closes itself only when it cannot read a response. We could port the reconnect wrapper from https://github.com/cs3org/reva/blob/edge/pkg/utils/ldap/reconnect.go
There was a problem hiding this comment.
I think this needs to be solved before this can be merged. One way or another the connection needs to be "checked" if it is still usable (or some retry logic).
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
butonic
force-pushed
the
reuse-ldap-connection
branch
from
4ee2b75 to
e9402eb
Compare
longsleep
left a comment
longsleep
left a comment
There was a problem hiding this comment.
Like I commented before, I think there needs to be one way or another to ensure that a reused LDAP connection still is usable to errors just because the connection dropped.
3 participants
@rhafer @longsleep this PR will reuse the global ldap connection and use a dedicated connection for user binds when checking passwords. This limits the amount of memory used when checking user passwords and allows all global requests to reuse the connection, saving the argon2 overhead for the global connection which happens when calling
ResolveUserByUsernameorGetUser.fixes owncloud/ocis#5765