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

forkJoin doesn't wait for last input's finally block #4914

Closed
deastr opened this issue Jul 16, 2019 · 4 comments · Fixed by #6546
Closed

forkJoin doesn't wait for last input's finally block #4914

deastr opened this issue Jul 16, 2019 · 4 comments · Fixed by #6546
Assignees
Labels
bug

Comments

@deastr
Copy link

@deastr deastr commented Jul 16, 2019

RxJS version: 6.5.2

Additional information:

forkJoin doesn't seem to wait for the last input's finally block when the input observables have finally blocks.

Code to reproduce:

  get(url: string): Observable<any> {
    return this.http.get(url).finally(() => console.log(url + ' completed.'));
  }

  bulkRequest(...sources: SubscribableOrPromise<any>[]): Observable<any[]> {
    console.log('bulkRequest start'); 
    return forkJoin(sources)
        .finally(() => console.log("forkjoin completed"));
  }

  ngAfterViewInit(){
    this.bulkRequest(this.get('https://jsonplaceholder.typicode.com/posts/1'),
    this.get('https://jsonplaceholder.typicode.com/posts/2'),
    this.get('https://jsonplaceholder.typicode.com/posts/3'),
    this.get('https://jsonplaceholder.typicode.com/posts/4'),
    this.get('https://jsonplaceholder.typicode.com/posts/5'),
    this.get('https://jsonplaceholder.typicode.com/posts/6'),
    this.get('https://jsonplaceholder.typicode.com/posts/7'),
    this.get('https://jsonplaceholder.typicode.com/posts/8'),
    this.get('https://jsonplaceholder.typicode.com/posts/9'),
    this.get('https://jsonplaceholder.typicode.com/posts/10'))
    .subscribe(results => {
      console.log('bulkRequest completed');
      for(let i = 0; i < results.length; i++) {
        console.log('result ' + (i + 1).toString() + ': ' + results[i].id);
      }
    });
  }

https://stackblitz.com/edit/angular-u3vqnu

Expected behavior:

bulkRequest start
https://jsonplaceholder.typicode.com/posts/1 completed.
https://jsonplaceholder.typicode.com/posts/2 completed.
https://jsonplaceholder.typicode.com/posts/3 completed.
https://jsonplaceholder.typicode.com/posts/4 completed.
https://jsonplaceholder.typicode.com/posts/5 completed.
https://jsonplaceholder.typicode.com/posts/6 completed.
https://jsonplaceholder.typicode.com/posts/7 completed.
https://jsonplaceholder.typicode.com/posts/8 completed.
https://jsonplaceholder.typicode.com/posts/9 completed.
https://jsonplaceholder.typicode.com/posts/10 completed.
bulkRequest completed
result 1: 1
result 2: 2
result 3: 3
result 4: 4
result 5: 5
result 6: 6
result 7: 7
result 8: 8
result 9: 9
result 10: 10
forkjoin completed

Actual behavior:

bulkRequest start
https://jsonplaceholder.typicode.com/posts/1 completed.
https://jsonplaceholder.typicode.com/posts/2 completed.
https://jsonplaceholder.typicode.com/posts/3 completed.
https://jsonplaceholder.typicode.com/posts/4 completed.
https://jsonplaceholder.typicode.com/posts/5 completed.
https://jsonplaceholder.typicode.com/posts/6 completed.
https://jsonplaceholder.typicode.com/posts/7 completed.
https://jsonplaceholder.typicode.com/posts/8 completed.
https://jsonplaceholder.typicode.com/posts/9 completed.
bulkRequest completed
result 1: 1
result 2: 2
result 3: 3
result 4: 4
result 5: 5
result 6: 6
result 7: 7
result 8: 8
result 9: 9
result 10: 10
forkjoin completed
https://jsonplaceholder.typicode.com/posts/10 completed.

The last "completed" one isn't always necesserily the 10th one, any other could be.

@cartant
Copy link
Collaborator

@cartant cartant commented Jul 16, 2019

This does not look like a bug, to me. The first thing to note is that the code logs bulkRequest completed from within the handler for a next notification and that notification will be emitted before completion.

@deastr
Copy link
Author

@deastr deastr commented Jul 16, 2019

So is this the order of execution:

  1. last http.get()
  2. bulkRequest.subscribe (which is forkJoin.subscribe I think?)
  3. forkJoin.finally
  4. last http.get().finally

@benlesh
Copy link
Member

@benlesh benlesh commented May 28, 2021

const source = forkJoin(
  [1, 2, 3, 4].map(n =>
    timer(100).pipe(
      map(() => n),
      finalize(() => console.log(`Finalized ${n}`))
    )
  )
);

source.subscribe(console.log);

Output:

Finalized 1
Finalized 2
Finalized 3
[1, 2, 3, 4]
Finalized 4

It's certainly surprising behavior, I guess. I mean, I know why it's happening, but I think it would be best if it were more predictable. IMO, this is a bug similar to some other issues I've fixed recently. And I can think of other places this is probably happening too.

@benlesh benlesh self-assigned this May 28, 2021
@benlesh benlesh added the bug label May 28, 2021
@cartant
Copy link
Collaborator

@cartant cartant commented May 28, 2021

but I think it would be best if it were more predictable

FWIW, this predictability - i.e. a simple/small set of rules - is one of the things I had in mind when we were discussing the take business. I really do think that all consumers should unsubscribe ASAP - i.e. before sending further notifications - but I don't think we can do this before v8. I mean, we can fix this issue, but I'm pretty sure it will pop up elsewhere and we still won't have a simple rule to reason with.

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

Successfully merging a pull request may close this issue.

3 participants