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

[Performance] Observable are not unsubscribed leading to higher CPU use #147

Closed
ng-gist opened this issue Mar 6, 2019 · 2 comments
Closed
Assignees
Labels
bug Something isn't working enhancement New feature or request priority Needs to be done ASAP
Projects
Milestone

Comments

@ng-gist
Copy link

ng-gist commented Mar 6, 2019

Describe the bug
Swapsteem code uses RxJS to subscribe to changes in data. RxJS in-turn use Observable which are Infinite Subscriptions. This means that the code will execute even without a subscribe being called unlike http.get where call will only be made when you subscribe to it. This causes a performance and memory hog as you exit and re-enter a controller because you are subscribing to same service multiple times. You can find more details in the stack overflow discussion here: https://stackoverflow.com/a/41177163/1141542

To Reproduce
This problem can be observed in dozens of places throughout the code. Below is a discussion on one such scenario and a fix for the same.

terms-and-conditions.component.ts

  this.auth.getUserData().subscribe((scAuthService) => {
        this.ngxService.stop();
        if (scAuthService) {
          this.auth.userData = scAuthService;
        }
        this.dialogRef.close();
      })

Expected behavior
Above code can be re-written as.

  this.auth.getUserData().pipe(takeWhile(() => this.isAlive)).subscribe((scAuthService) => {
        this.ngxService.stop();
        if (scAuthService) {
          this.auth.userData = scAuthService;
        }
        this.dialogRef.close();
      })

When controller destroys set isAlive to false

  ngOnDestroy() {
    this.isAlive = false;
  }
@nirvanaitsolutions
Copy link
Owner

Cool hunt! Thank you for letting us know about this. We have not been doing code based on performance till now, but this hunt will make us focus on that aspect. We accept it as a valid contribution and will fix ASAP.
Thanks again!

@nirvanaitsolutions nirvanaitsolutions added bug Something isn't working enhancement New feature or request priority Needs to be done ASAP labels Mar 7, 2019
@nirvanaitsolutions nirvanaitsolutions added this to To do in swapsteem via automation Mar 7, 2019
@nirvanaitsolutions nirvanaitsolutions added this to the accepted milestone Mar 7, 2019
@nirvanaitsolutions nirvanaitsolutions moved this from To do to In progress in swapsteem Mar 14, 2019
@nirvanaitsolutions nirvanaitsolutions moved this from In progress to Needs review in swapsteem Mar 14, 2019
@nirvanaitsolutions nirvanaitsolutions moved this from Needs review to In progress in swapsteem Mar 14, 2019
@nirvanaitsolutions
Copy link
Owner

Fixed

@AneilPatel05 AneilPatel05 moved this from In progress to Reviewer approved in swapsteem May 11, 2019
@AneilPatel05 AneilPatel05 moved this from Reviewer approved to beta in swapsteem May 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request priority Needs to be done ASAP
Projects
swapsteem
  
beta
Development

No branches or pull requests

4 participants