Skip to content

Improve docstring for channels#release and add log #2190

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.

Sign up for GitHub

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

Open
SimonWoolf wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −5
Open

Improve docstring for channels#release and add log #2190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ably.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2718,7 +2718,7 @@ export declare interface Channels<T> {
*/
getDerived(name: string, deriveOptions: DeriveOptions, channelOptions?: ChannelOptions): T;
/**
* Releases a {@link Channel} or {@link RealtimeChannel} object, deleting it, and enabling it to be garbage collected. To release a channel, the {@link ChannelState} must be `INITIALIZED`, `DETACHED`, or `FAILED`.
* Releases all SDK-held references to a {@link Channel} or {@link RealtimeChannel} object, enabling it to be garbage collected. Warning: this method has no guardrails; using a channel reference after it has been released is undefined behaviour. It can be useful for applications that work with a continually changing set of channels on a single client and need to avoid unbounded memory growth; if this does not describe you, don't call it. Realtime channels not already in the `INITIALIZED`, `DETACHED`, or `FAILED` state are detached before release.
*
* @param name - The channel name.
*/
Expand Down
22 changes: 18 additions & 4 deletions src/common/lib/client/baserealtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,15 +223,29 @@ class Channels extends EventEmitter {
* Please do not use this unless you know what you're doing */
release(name: string) {
name = String(name);
Logger.logAction(this.logger, Logger.LOG_MAJOR, 'Channels.release()', 'Releasing references to channel ' + name);
const channel = this.all[name];
if (!channel) {
return;
}
const releaseErr = channel.getReleaseErr();
if (releaseErr) {
throw releaseErr;
const s = channel.state;
if (s === 'initialized' || s === 'detached' || s === 'failed') {
delete this.all[name];
return;
}
delete this.all[name];
channel
Copy link
Copy Markdown
Contributor

@AndyTWF AndyTWF Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this necessitate a major version bump as we're changing the behaviour? Is that the plan?

Copy link
Copy Markdown
Member Author

@SimonWoolf SimonWoolf Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a semantics change, but in the direction of being strictly more permissive, which makes it a backwards-compatible change. If someone who was using the function before without it throwing would continue to work. (and note how it would be naturally async now but I did the slightly-awkward .catch().then() so that I'm not changing the signature of the function to return a promise).

This is why I went this route rather than changing ably-java etc to be stricter, which I would have preferred, but that would require a major version bump.

(in some sense, any observable behaviour change could be relied on by someone: it's not literally impossible that someone is relying on the behaviour that release() throws for an attached channel. but it's pretty damn unlikely, and an interpretation of semver where such a change would require a major version bump would imply there can be no such thing as a minor or patch bump. every code change other than pure refactor causes some observable behaviour change or there would be no point doing it, anyone can rely on anything. so ultimately it has to be a judgement over what people are likely to be relying on. cf https://xkcd.com/1172/ )

.detach()
.catch((err) => {
Logger.logAction(
this.logger,
Logger.LOG_ERROR,
'Channels.release()',
'Error detaching channel ' + name + ' prior to release: ' + Utils.inspectError(err),
);
})
.then(() => {
delete this.all[name];
});
}
}

Expand Down
Loading