Skip to content
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

Detecting when a connection has been terminated #395

Open
hartfordfive opened this issue Aug 31, 2022 · 7 comments
Open

Detecting when a connection has been terminated #395

hartfordfive opened this issue Aug 31, 2022 · 7 comments
Labels

Comments

@hartfordfive
Copy link

hartfordfive commented Aug 31, 2022

I have a use-case where I'm opening a connection that may be long-lived without any activity for a given period of time. Unfortunately it seems that these connections become "stale" sometimes and need to be re-established. Is there a method I can used directly, prior to running the ldap.NewSearchRequest function (in the v3 package) to confirm if the connection is still active/healthy, and if not, re-connect?

@cpuschma
Copy link
Member

When the connection has been closed, the goroutines responsible for handling ingress and egress internally call conn.setClosing. You can retrieve the state and check if the reader has been stopped by a closing connection by calling conn.IsClosing:

ldap/v3/conn.go

Lines 258 to 261 in 93825e9

// IsClosing returns whether or not we're currently closing.
func (l *Conn) IsClosing() bool {
return atomic.LoadUint32(&l.closing) == 1
}

However, this does not account to abrupt connection losses, there's no way to determine if a connection has been forcefully killed without trying to send anything over the wire or wait for TCP timeout to happen.

@iredmail
Copy link
Contributor

hi @cpuschma

Can we add some code to detect and automatically reconnect?

My case is, if openldap server was restarted, the connection established by go-ldap module will become useless and never reconnect, so all further ldap queries are failed. Restarting Go application fixes the issue but this is not idea at all.

I used Python + python-ldap module before, its ReconnectLDAPObject works well in such case:
https://www.python-ldap.org/en/python-ldap-3.4.3/reference/ldap.html#ldap.ldapobject.ReconnectLDAPObject

@eryajf
Copy link

eryajf commented Nov 19, 2022

@iredmail I recommend you try this library: https://github.com/eryajf/ldapool

@cpuschma
Copy link
Member

hi @cpuschma

Can we add some code to detect and automatically reconnect?

My case is, if openldap server was restarted, the connection established by go-ldap module will become useless and never reconnect, so all further ldap queries are failed. Restarting Go application fixes the issue but this is not idea at all.

I used Python + python-ldap module before, its ReconnectLDAPObject works well in such case: https://www.python-ldap.org/en/python-ldap-3.4.3/reference/ldap.html#ldap.ldapobject.ReconnectLDAPObject

Hi,

for me this is not scope of the project. I have already told you above everything necessary to check if the connection was terminated. You only have to implement this check and establish a new connection in case of an error.

I have the opinion that the library should not take over the decision of the users or developers, but should provide the basic functionality for them. For the same reason, the library does not have its own function for connection pooling, for example, because there are different ways to implement this and we block the way for us and the users because of backward compatibility.

@iredmail
Copy link
Contributor

iredmail commented Nov 20, 2022

My personal opinion: if the feature is required by most users, then better implement it in the library directly to bring the usability to a higher level. Otherwise everyone uses this ldap library has to implement his own pool and auto reconnection, it's a waste of time.

@cpuschma
Copy link
Member

cpuschma commented Oct 31, 2024

I understand where you're coming from, but it's not a waste of time, since multiple things have to be considered which are highly dependant on the scenario:

  1. there would be a lot to consider, which extremely depends on the application scenario, since LDAPv3 is a connection-oriented protocol and you cannot use different connections for the same operation (e.g. paging cookies, consecutive message IDs)

  2. there are several potential approaches on how pooling could be implemented. Either you agree on one way and give the users the way or you implement all ways but have the corresponding overhead. For example: a FIFO queue using channels or arrays, Least Used Connection, both combined with separation according to Authz and Authn

  3. Our CONTRIBUTING.md clearly states:

  • beware of baking in implicit behavior based on other libraries/tools choices
  • be as high-fidelity as possible in plumbing through LDAP data (don't mask errors or reduce power of someone using the library)

As a developer, I clearly prefer the way where a library gives me all the possibilities to implement what I need. go-ldap forms the basis for exactly that.

But I can imagine a scenario, for example, where we open a third repository under the “go-ldap” organization that implements exactly this, so that the “ldap” repository is not flooded with potential pooling implementations.

@johnweldon any thoughts? I would like to implement it in the main package and either put it in a separate folder or completely in a separate repository.

@johnweldon
Copy link
Member

I'm skeptical of requests to implement something that the requester hasn't validated or even tried to implement themselves.

At least @eryajf has implemented their own solution to this problem. I haven't personally looked at that implementation, but if it turns out to be useful and robust, the license is compatible, and the maintainer is willing we could explore working closer with them.

If you want to implement your own version, I'm very supportive as long as care is taken to avoid negatively impacting the existing value of the library. So in a separate package is good, and even a different repo might be better.

All that to say, whatever you're willing to work on I'm happy to support @cpuschma - my attention hasn't been very available for this project, but I greatly appreciate your involvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants