#1507 Investigate terminating connections in sdap_ops.c and comment the code some more
Opened 6 years ago by jhrozek. Modified 2 years ago

We removed a piece of code that was causing a double-free in 52828e4.

During the review, the following was recommended by the original developer:

The main point of this code was to prevent deleting the connection every time  
operation ends. The patch introducing the disconnection flag should be solving 
two major things:

Don't accept new operations for the connection that is disconnecting. This is  
solved in another part of the code.

Don't free the connection immediatelly when you detect it's not working. 
Rather mark it as disconnecting and let all operations on that connection 
finish (probably with an error). Only the last finished operation should be 
deleting the connection. Otherwise you will sometimes get hanging operations.  
IIRC, that's what I wanted to solve by this hunk of the code. I probably 
didn't notice the code deleting connection every time the operation ends.

We should investigate the above and possibly fix the disconnection logic.

At the same time, it is evident that this whole module is hard to
understand. We should make sure it is more readable by introducing more
comments or (careful) refactoring.


We need to investigate this code more. A separate ticket should be filed to add more comments to the code.

description: We removed a piece of code that was causing a double-free in 52828e4.

During the review, the following was recommended by the original developer:
{{{
The main point of this code was to prevent deleting the connection every time
operation ends. The patch introducing the disconnection flag should be solving
two major things:

Don't accept new operations for the connection that is dicsonnecting. This is
solved in another part of the code.

Don't free the connection immediatelly when you detect it's not working.
Rather mark it as disconnecting and let all operations on that connection
finish (probably with an error). Only the last finished operation should be
deleting the connection. Otherwise you will sometimes get hanging operations.
IIRC, that's what I wanted to solve by this hunk of the code. I probably
didn't notice the code deleting connection every time the operation ends.
}}}

We should investigate the above and possibly fix the disconnection logic.

At the same time, it is evident that this whole module is hard to
understand. We should make sure it is more readable by introducing more
comments or (careful) refactoring.
=> We removed a piece of code that was causing a double-free in 52828e4.

During the review, the following was recommended by the original developer:
{{{
The main point of this code was to prevent deleting the connection every time
operation ends. The patch introducing the disconnection flag should be solving
two major things:

Don't accept new operations for the connection that is disconnecting. This is
solved in another part of the code.

Don't free the connection immediatelly when you detect it's not working.
Rather mark it as disconnecting and let all operations on that connection
finish (probably with an error). Only the last finished operation should be
deleting the connection. Otherwise you will sometimes get hanging operations.
IIRC, that's what I wanted to solve by this hunk of the code. I probably
didn't notice the code deleting connection every time the operation ends.
}}}

We should investigate the above and possibly fix the disconnection logic.

At the same time, it is evident that this whole module is hard to
understand. We should make sure it is more readable by introducing more
comments or (careful) refactoring.

milestone: NEEDS_TRIAGE => SSSD 1.9.1
rhbz: => 0
type: defect => task

Fields changed

milestone: SSSD 1.9.1 => SSSD 1.9.2

Fields changed

owner: somebody => okos
status: new => assigned

Fields changed

milestone: SSSD 1.9.2 => SSSD 1.9.3

Not critical for 1.9.3

design: =>
design_review: => 0
fedora_test_page: =>
milestone: SSSD 1.9.3 => SSSD 1.9.4

Dropping the investigation/documentation tasks to trivial. These can be deferred if needed.

priority: major => trivial

Moving docs task to 1.9.5

milestone: SSSD 1.9.4 => SSSD 1.9.5

Fields changed

milestone: SSSD 1.9.5 => SSSD 1.11 beta
review: => 0
selected: =>

Fields changed

changelog: =>
milestone: SSSD 1.13 beta => Interim Bucket
priority: trivial => major

Fields changed

milestone: Interim Bucket => SSSD 1.12 beta

Should be done with the LDAP refactoring planned for 1.13

milestone: SSSD 1.12 beta => SSSD 1.13 beta

Fields changed

mark: => 0

This is a prerequisite of refactoring the LDAP provider. We need to have the whole codebase documented first.

milestone: SSSD 1.13 beta => SSSD 1.13 backlog

Mass-moving tickets not planned for the 1.13 release to 1.14

milestone: SSSD 1.13 backlog => SSSD 1.14 beta

The sdap_ops.c module still needs a lot of work..

milestone: SSSD 1.14 beta => SSSD 1.15 beta
sensitive: => 0

Fields changed

milestone: SSSD 1.16 beta => SSSD 1.15 Alpha
owner: okos => somebody
status: assigned => new

Fields changed

milestone: SSSD 1.15.0 => SSSD 1.15 Beta

Fields changed

milestone: SSSD 1.15.3 => SSSD 1.15.2

Metadata Update from @jhrozek:
- Issue marked as blocked by: #2976
- Issue set to the milestone: SSSD 1.15.2

2 years ago

Metadata Update from @jhrozek:
- Custom field design_review reset
- Custom field mark reset
- Custom field patch reset
- Custom field review reset
- Custom field sensitive reset
- Custom field testsupdated reset

2 years ago

Metadata Update from @jhrozek:
- Custom field design_review reset
- Custom field mark reset
- Custom field patch reset
- Custom field review reset
- Custom field sensitive reset
- Custom field testsupdated reset
- Issue set to the milestone: SSSD Future releases (no date set yet) (was: SSSD 1.15.2)

2 years ago

Login to comment on this ticket.

Metadata