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

first() produce a lot more than 1 data in some recursive situation. #2098

Closed
kasiditi opened this issue Nov 1, 2016 · 4 comments · Fixed by #2100
Closed

first() produce a lot more than 1 data in some recursive situation. #2098

kasiditi opened this issue Nov 1, 2016 · 4 comments · Fixed by #2100
Assignees

Comments

@kasiditi
Copy link
Contributor

kasiditi commented Nov 1, 2016

It is pretty hard to describe in text. The code below might be more understandable.

I have a Subject that is connected with first() and subscribed with a observer that will fire a data synchronously to the same Subject with next(). It turns out that the observer receives a lot of data even though it is chained with first().

I found this bug in more personal complicated code, but below is a simplified repro one. Also, I'm not sure if this is expected behavior or not.

RxJS version:
5.0.0-beta.12

Code to reproduce:

const valueSubject = new Subject();
const result = [];

valueSubject.first().subscribe(val => {
    result.push(val);
    valueSubject.next(false);
});

valueSubject.next(true);

// Expect result.length to be 1.

Expected behavior:
Data should be received once.

Actual behavior:
Large amount of data fired to observer. Sometimes, the Error Maximum call stack size exceeded shows up.

Additional information:
When data is pushed asynchronously, the test will pass. Here is the asynchronous test in Jasmine:

it('should behave correctly', (done) => {
    const valueSubject = new Subject<boolean>();
    const result: boolean[] = [];

    valueSubject.first().subscribe(val => {
        result.push(val);
        setTimeout(() => valueSubject.next(false), 0);
    });

    valueSubject.next(true);

    setTimeout(() => {
        expect(result.length).toBe(1);
        done();
    }, 100);
});

In this case, the test passes.

@kwonoj
Copy link
Member

kwonoj commented Nov 1, 2016

I think this is similar case to #1759 (comment) to try synchronous reentrant causes those behavior. As your last snippet does, if you explicitly make it asynchronous to trap out from synchronous boundary it'll work, as similar observeOn or other different scheduling approach does.

/cc @trxcllnt to confirm in case if I understood this incorrectly and this is unexpected behavior.

@trxcllnt
Copy link
Member

trxcllnt commented Nov 2, 2016

Yes we should likely guard after the first emission, similar in spirit to #1967

@benlesh benlesh self-assigned this Nov 3, 2016
@benlesh
Copy link
Member

benlesh commented Nov 3, 2016

I'll have a look

jayphelps pushed a commit that referenced this issue Nov 5, 2016
adds guard after first emitted value to prevent reentrant behavior

fixes #2098
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants