go-ldap / ldap

Basic LDAP v3 functionality for the GO programming language.
Other
2.19k stars 352 forks source link

v3 and root copies of the code desynced #520

Closed maxb closed 1 month ago

maxb commented 1 month ago

I think this is unintended:

--- ./conn.go   2024-05-27 20:22:00.225638254 +0000
+++ v3/conn.go  2024-05-27 20:22:00.229638338 +0000
@@ -535,9 +535,10 @@
                                l.messageContexts[message.MessageID] = message.Context

                                // Add timeout if defined
-                               if l.getTimeout() > 0 {
+                               requestTimeout := l.getTimeout()
+                               if requestTimeout > 0 {
                                        go func() {
-                                               timer := time.NewTimer(time.Duration(l.getTimeout()))
+                                               timer := time.NewTimer(time.Duration(requestTimeout))
                                                defer func() {
                                                        if err := recover(); err != nil {
                                                                l.err = fmt.Errorf("ldap: recovered panic in RequestTimeout: %v", err)
johnweldon commented 1 month ago

What is your concern @maxb ? This seems like a reasonable change.

johnweldon commented 1 month ago

Ah, you're saying there's a difference between the v3/conn.go and the ./conn.go

johnweldon commented 1 month ago

I see @cpuschma caught this; we'll need to update the ./conn.go to match the v3/conn.go I suppose.

maxb commented 1 month ago

Quite... I wanted to open the can of worms of do we really need to continue having two copies of the code in the repo (in a separate issue), but making sure the two copies really are just duplicates is a necessary prerequisite.

cpuschma commented 1 month ago

We are already "discussing" our options regarding the two copies (see https://github.com/orgs/go-ldap/discussions/503). It is clear, and the incident has made it clear once again, that we have to stop doing this in the long term.

Thank you for pointing out the problem!