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

#8843 Clean up request data references #8883

Merged
merged 4 commits into from
Jun 3, 2020
Merged

#8843 Clean up request data references #8883

merged 4 commits into from
Jun 3, 2020

Conversation

bn-dignitas
Copy link
Contributor

@bn-dignitas bn-dignitas commented May 28, 2020

Fix for #8843 Clear Request cancelFunction and deferred references to free up unintentional retained request data memory

… unintentional retained request data memory
@cesium-concierge
Copy link

Thanks for the pull request @bn-dignitas!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@OmarShehata OmarShehata requested a review from mramato May 28, 2020 19:47
@hpinkos
Copy link
Contributor

hpinkos commented May 29, 2020

@mramato I think this is ready. Did you have any other comments? Should we add a note to CHANGES.md?

@mramato
Copy link
Contributor

mramato commented May 29, 2020

Sorry, I meant to tag @lilleyse since I think he was the one that originally confirmed the issue was request/tile related and knows this area of the code. Sean?

@lilleyse
Copy link
Contributor

I should have a chance to review later tonight.

@lilleyse
Copy link
Contributor

Looks good to me. Nothing to add from what @mramato said. I opened a companion PR to fix this at the Heap/ManagedArray level in case other systems begin to use these files. #8889

@itsegg could you check if this branch fixes your issue in #4549 (comment)?

@itsegg
Copy link

itsegg commented Jun 1, 2020

@lilleyse
I think it's ok,
I have tested bn-dignitas:8843-fix-request-memory-retention,
My problem can be solved effectively.

@lilleyse
Copy link
Contributor

lilleyse commented Jun 3, 2020

Thanks @bn-dignitas!

@lilleyse lilleyse merged commit 71c5250 into CesiumGS:master Jun 3, 2020
@lilleyse
Copy link
Contributor

lilleyse commented Jun 3, 2020

Updated CHANGES.md in master: 2abec98

@@ -182,6 +182,8 @@ function getRequestReceivedFunction(request) {
requestCompletedEvent.raiseEvent();
request.state = RequestState.RECEIVED;
request.deferred.resolve(results);
// explicitly set to undefined to ensure GC of request response data. See #8843
request.deferred = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

@lilleyse I have a regression introduced by this change, but it may be an unrelated bug that this is just covering up, not sure. Either way it's a RequestScheduler issue. I'm trying to create a simple reproduction now and then I'll write it up. Hopefully it's an easy fix (since you know this code way better than I do).

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.

6 participants