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

Remove unusable key sessions when ProtectionController is stopped wit… #4239

Merged
merged 2 commits into from
Jul 26, 2023

Conversation

dsilhavy
Copy link
Collaborator

…hout waiting for session.close promise to be resolved. Addresses #4238

@dsilhavy dsilhavy added this to the 4.7.2 milestone Jul 24, 2023
@dsilhavy dsilhavy merged commit 4b4a8b1 into development Jul 26, 2023
@dsilhavy dsilhavy deleted the fix/#4238 branch July 26, 2023 05:38
@alexhmit
Copy link

alexhmit commented Apr 4, 2024

The problem looks worse than we first thought:

for (let i = 0; i < numSessions; i++) {
                session = sessions[i];
                (function (s) {
                    _closeKeySessionInternal(session)
                    done(s);
                })(session);
  }

The done() calls removeSession which can splice sessions. This can make the next sessions[i] in the for loop to reference the wrong or undefine array item. Basically this code would not work for sessions.length > 1.

@dsilhavy
Copy link
Collaborator Author

dsilhavy commented Apr 4, 2024

@alexhmit can you provide a fix for this?

@alexhmit
Copy link

alexhmit commented Apr 9, 2024

Sorry about the late response. For some reason, I am not getting any notification of this ticket.

We patched temporarily as follow:

            for (let i = numSessions - 1; i >= 0; i--) {
                session = sessions[i];
                (function (s) {
                    _closeKeySessionInternal(session)
                    done(s);
                })(session);
            }

Basically reversing the order of sessions processing so any splicing on sessions won't affect the looping. We did this to minimize the changes to the original code. We are not sure if there are other area of codes afflicted with this problem. But it fixed our regression problems.

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

Successfully merging this pull request may close these issues.

2 participants