Conversation
…oved at Firebase RDB side.
…own subscriptions. Remove debug logging as well.
|
@YegorUdovchenko @dmitrykuzmin since you are the original reporters, please have a look. |
|
@YegorUdovchenko @dmitrykuzmin in scope of the following PRs, I am going to update the Firebase Admin SDK to the latest 9.1.x version. This will affect the Java API of |
We'll have to version our server API during this migration anyway, so the Admin SDK upgrade is not a big deal. |
YegorUdovchenko
left a comment
YegorUdovchenko
left a comment
There was a problem hiding this comment.
LGTM. Please see some comments.
| return new EventSubscription({ | ||
| unsubscribedBy: () => { | ||
| FirebaseSubscribingClient._unsubscribe([pathSubscription]); | ||
| return this._subscriptionService.cancelSubscription(subscription); |
There was a problem hiding this comment.
There's no need to return here, cancelSubscription is void.
There was a problem hiding this comment.
Well-spotted! I have removed this return just in another use-case, and completely forgot about this one.
| cancelSubscription(subscription) { | ||
| this._endpoint.cancelSubscription(subscription) | ||
| .then(() => { | ||
| this._removeSubscription(subscription) |
There was a problem hiding this comment.
Here subscription is a Protobuf message, _removeSubscription expects something derived from RxJS Subscription. I guess we should rework _removeSubscription method so it could remove subscriptions by ID.
There was a problem hiding this comment.
@YegorUdovchenko
That's an interesting point. Perhaps, you are right.
However, I don't understand how this thing worked previously. It uses subscription.internal() objects, which AFAIU are Protobuf messages.
There was a problem hiding this comment.
I don't think it actually works.
There was a problem hiding this comment.
@YegorUdovchenko
I have reworked this removal thing to take any kind of Subscription, and operate by its ID. I think, that's the safest approach for now.
…ription` types by using their IDs. Improve the formatting of type-level docs.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## v1 #195 +/- ##
============================================
- Coverage 63.17% 62.40% -0.78%
- Complexity 214 215 +1
============================================
Files 96 96
Lines 2724 2761 +37
Branches 46 47 +1
============================================
+ Hits 1721 1723 +2
- Misses 992 1027 +35
Partials 11 11 |
3 participants
This PR addresses #188.
In particular, the following changes were made to the behaviour:
Calling
unsubscribefor any subscription now leads to an immediate cancellation on both client- and server-side. Previously, all such cancellations were only processed during the next "keep-up" propagation.Clientnow allows cancelling all known subscription viacancelAllSubscriptions()no-args call. Its invocation leads to sending the bulk cancellation request to server-side, as well as shutting down the client-level subscriptions. Such an API is useful in case end-users choose to log out from the application.Also, this PR aims to improve the server-side memory consumption.
In particular, server-side holds a couple of internal registries to track active subscriptions. Previously, upon cancellation of a subscription, some of these registries were not affected, thus producing a memory leak. Now, each cancelled subscription is removed from all in-memory caches.
In future PRs there will be more updates to this repository, addressing the library updates available for both Java and client-side code.
The library version is set to
1.9.0-SNAPSHOT.10.