Conversation
| | iterate() -> `Iterator<ChannelType>` ||| RSN2, RTS2 | Iterates through the existing channels. | | ||
| ||| `ChannelType` || Each iteration returns a [`RestChannel`]{@link RestChannel} or [`RealtimeChannel`]{@link RealtimeChannel} object. | | ||
| | release(String) ||| RSN4, RTS4 | Releases a [`RestChannel`]{@link RestChannel} or [`RealtimeChannel`]{@link RealtimeChannel} object, deleting it, and enabling it to be garbage collected. It also removes any listeners associated with the channel. To release a channel, the [`ChannelState`]{@link ChannelState} must be `INITIALIZED`, `DETACHED`, or `FAILED`. | | ||
| | release(String) ||| RSN4, RTS4 | Releases all SDK-held references to a [`RestChannel`]{@link RestChannel} or [`RealtimeChannel`]{@link RealtimeChannel} object, enabling it to be garbage collected. This is exposed for certain niche usecases involving using a continually changing set of channels with a client without memory usage increasing unboundedly. Most users will never need to use this and should ignore it. Keeping a reference to a channel and calling methods on it after it has been `release()`d is undefined behaviour. To release a channel its [`ChannelState`]{@link ChannelState} must be `INITIALIZED`, `DETACHED`, or `FAILED`. | |
There was a problem hiding this comment.
Agree with the undefined behaviour part - though I would question if it's "niche".
I think in some chat use-cases this is quite common, especially where React is concerned and you may be bringing channels in-and-out of scope regularly in a useEffect hook.
There was a problem hiding this comment.
I think in some chat use-cases this is quite common, especially where React is concerned and you may be bringing channels in-and-out of scope regularly in a useEffect hook.
Why would bringing channels into and out of scope mean you need to release them? Are people using one chat client cycling through thousands of channels? I've only ever seen serverside uses (which might need to manage or publish into many clients) need to call release
...also I've just looked through and found ably/docs#1057 (comment) where apparently we decided to change the spec to make release() implicitly detach first, but then didn't do that in ably-js? aargh
There was a problem hiding this comment.
Why would bringing channels into and out of scope mean you need to release them?
Guess if you had a support-agent style chat where lots of chats come and go, you would want to fully dispose the resource once a chat is done, not just detach the channel.
but then didn't do that in ably-js? aargh
Yup, we do it in ably-java D:
There was a problem hiding this comment.
shrug, fair enough -- I guess if there's an additional sdk layer guaranteeing that no-one's going to try to use the channel again after release it's fine.
ok, so, i guess change this pr to remove the bit about the required channel state, and make the ably-js detach on release. But I'd still rather err on the side of the docstring telling people not to use it, it was always intended for advanced users and doesn't have much in the way of guardrails. (...probably we should add guardrails and prevent use after release, but, well, currently we don't).
There was a problem hiding this comment.
@AndyTWF I've updated this pr, the ably-js pr, and the docs pr per the RTS4a behaviour.
(honestly I would prefer an error, this is a dangerous method and I think making it automagic is a step in the wrong direction, but all other SDKs implement the other behaviour and can't be changed to the ably-js semantics without a breaking change, so, shrug, this is the simplest way to uniformity)
SimonWoolf
force-pushed
the
release-clarify
branch
from
f0f5022 to
7674499
Compare
2 participants
The current docstring almost makes it sound like we expect people to call release(). 99% of people should not. There's a reason the original ably-js comment on this method was
/* Included to support certain niche use-cases; most users should ignore this. Please do not use this unless you know what you're doing */