#1507 Investigate terminating connections in sdap_ops.c and comment the code some more
Closed: wontfix 4 years ago by pbrezina. Opened 11 years ago by jhrozek.

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

7 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

7 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)

7 years ago

Metadata Update from @thalman:
- Custom field design_review reset (from false)
- Custom field mark reset (from false)
- Custom field patch reset (from false)
- Custom field review reset (from false)
- Custom field sensitive reset (from false)
- Custom field testsupdated reset (from false)
- Issue tagged with: Canditate to close

4 years ago

Thank you for taking time to submit this request for SSSD. Unfortunately this issue was not given priority and the team lacks the capacity to work on it at this time.

Given that we are unable to fulfill this request I am closing the issue as wontfix.

If the issue still persist on recent SSSD you can request re-consideration of this decision by reopening this issue. Please provide additional technical details about its importance to you.

Thank you for understanding.

Metadata Update from @pbrezina:
- Issue close_status updated to: wontfix
- Issue status updated to: Closed (was: Open)

4 years ago

SSSD is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in SSSD's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/SSSD/sssd/issues/2549

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Login to comment on this ticket.

Metadata