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

Fix for multi-seg-in-flight #1782

Merged
merged 3 commits into from
Mar 23, 2021
Merged

Fix for multi-seg-in-flight #1782

merged 3 commits into from
Mar 23, 2021

Conversation

darkdarkdragon
Copy link
Contributor

@darkdarkdragon darkdarkdragon commented Mar 2, 2021

What does this pull request do? Explain your changes. (required)
Fixes case where data in SegsInFlight array gets lost after session refresh

Specific updates (required)

  • Add unit test showing bug
  • Add fix - after session update new session object should be assigned to lastSess field.

How did you test each of these updates (required)
Unit test.

Does this pull request close any open issues?

Checklist:

  • README and other documentation updated
  • Node runs in OSX and devenv
  • All tests in ./test.sh pass

@darkdarkdragon darkdarkdragon requested a review from jailuthra March 2, 2021 22:26
@darkdarkdragon
Copy link
Contributor Author

@jailuthra can you take a look? I think in case of call to refreshSession data in SegsInFlight gets lost...

@darkdarkdragon darkdarkdragon force-pushed the it/potential-problem branch 2 times, most recently from 4201123 to 6d7cd2b Compare March 23, 2021 04:56
'SegsInFlight' is lost in case of session refresh
@darkdarkdragon darkdarkdragon changed the title Potential multi-seg-in-flight problem Fix for multi-seg-in-flight Mar 23, 2021
@darkdarkdragon darkdarkdragon requested review from yondonfu and jailuthra and removed request for jailuthra March 23, 2021 08:26
@darkdarkdragon darkdarkdragon marked this pull request as ready for review March 23, 2021 08:26
Copy link
Contributor

@jailuthra jailuthra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed response.
Good catch & LGTM!

EDIT: Before merge it would be amazing if we can update the pending Changelog file for v0.5.16 eaf986b

@darkdarkdragon darkdarkdragon merged commit ffabc35 into master Mar 23, 2021
@darkdarkdragon
Copy link
Contributor Author

@jailuthra sorry I saw your comment about Changelog right after I've pressed 'merge' button

jailuthra added a commit that referenced this pull request Mar 24, 2021
@jailuthra jailuthra mentioned this pull request Mar 24, 2021
1 task
@yondonfu yondonfu deleted the it/potential-problem branch March 25, 2021 02:22
jailuthra added a commit that referenced this pull request Mar 25, 2021
jailuthra added a commit that referenced this pull request Mar 25, 2021
jailuthra added a commit that referenced this pull request Mar 25, 2021
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