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

It maybe !observer.closed => observer.closed when set returnObservable=true #836

Closed
lyhanburger opened this issue May 19, 2023 · 5 comments

Comments

@lyhanburger
Copy link

if (!observer.closed) return client.close()

it seems like

if (observer.closed)  return client.close();

because when call the unsubscribe() the closed set be true.

current code, make me can not close the grpc streamimg response

@stephenh
Copy link
Owner

Hi @KiyomiHan , I'm not sure and honestly am not really following; @hersentino I think wrote the code you linked to; Francois lmk if you have any thoughts on ^. Thanks!

@hersentino
Copy link
Contributor

hersentino commented May 22, 2023

Hi,

I'm not sure if this condition is the real issue here. I've tried removing it locally and used the unsubscribe function, but it seems like the add callback is never triggered. @KiyomiHan Can you test this on your end to confirm whether it works without the condition or not? You can use yalc to easily link your local ts-proto project to your project.

I initially added this condition to prevent the close function from being called again if it has already been closed by the abort signal system. So, the condition can safely be removed, but as I mentioned earlier, I don't think this will resolve your problem.

I'm not an expert with observable, but I believe the issue here is that the add callback is not being called even when the subscription is unsubscribed. I'm trying to figure out why. The add callback was already implemented when I introduced the abort signal system. I'm not sure if it was working or not at that time

Why don't you use the abort signal system instead of the unsubscribe method? The abort signal system is functioning properly

@hersentino
Copy link
Contributor

hersentino commented May 22, 2023

Okay, I made a mistake in my previous comment. The add callback works absolutely fine!
I am able to reproduce the bug locally. Additionally, by removing my condition, an abort is successfully triggered when the 'unsubscribe' method is called.

Sorry about the confusion. I'm going to submit a pull request today

@hersentino
Copy link
Contributor

The added condition to the pr is required, If I remove the condition, an error is triggered when the stream is aborted. This is why I initially wrote the faulty condition

Screenshot 2023-05-22 at 14 32 43

This error occurs because the close event is triggered twice: once by the abort event and once by the unsubscribe event.

With this pull request, everything works fine, both with and without the useAbortSignal option. I have tested both cases locally.

@stephenh
Copy link
Owner

This should be fixed by #837. Thanks @hersentino !

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

No branches or pull requests

3 participants