-
Notifications
You must be signed in to change notification settings - Fork 4
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
publish method on the Realtime (and REST) client object #64
Comments
… instancing a Channel object See https://github.com/ably/docs/issues/468
Hi, I think the wasted memory allocation should be released if it became inactive after some time. Or right now it's an issue to us that the memory keeps increasing just like memory leak on our node.js server. |
I am afraid we can't do that. We don't know what references people have to the object, and we also could potentially cause serious issues if a user has configured a channel with options that are then lost next time the channel is instanced.
So why not release the channels you no longer need explicitly? |
… instancing a Channel object See https://github.com/ably/docs/issues/468
… instancing a Channel object See https://github.com/ably/docs/issues/468
Building on the idea that you can publish without instancing a channel should also be added to the REST lib in IMHO. Customers have asked for this in the past, and more recently again, and it makes sense when in most cases, developers just need to publish to a channel with no channel options, either over REST or Realtime. @paddybyers @SimonWoolf I would like to consider adding this into the 1.2 release. |
… instancing a Channel object See https://github.com/ably/docs/issues/468
@QuintinWillison @paddybyers @SimonWoolf any further thoughts about this going into the 1.2 spec or not? |
In the case of both REST and Realtime this feels like a client-side convenience wrapper around existing functionality. However, I'm not familiar enough with the protocol yet to know whether there's a hidden implication I'm missing. |
Yes, that's correct. |
It is, but it's also a little more than that. Creating channel objects and creating references to these channel objects is a bit unnecessary in situations where a server, for example, is used to publish lots of messages on lots of varying channels. In the worst case this can lead to leaks as users don't free up those references as well. |
See https://app.intercom.com/a/apps/ua39m1ld/inbox/inbox/all/conversations/38483300001169, this customer is still relying on a callback to manage memory leaks with the channel object. See below. |
➤ Automation for Jira commented: The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-2809 |
We now support transient publishes on channels, however this API is somewhat counterintuitive if a user simply wants to stream lots of messages across lots of arbitrary channels to Ably. Whilst it's fair to say this is going to get complicated when users need to set channel options, I expect the vast majority of users don't use channel options, and instead have to do something like this:
Unless I am mistaken, that approach will result in channelsfoo
andbar
remaining attached (for publishing) using up resources within the Ably platform, as well as in the client.Correction: the client may instance a channel object, but realtime does not send an
ATTACHED
message, so the only wasted resources (objects that are not needed and not disposed) are client-side.A more natural and less resource intensive API may look something like:
It should be trivial to add this given we already support transient publishes.
Perhaps for simplicity we could not allow channel options for
client.publish
and simply state where channel options are needed, we recommend you use the "normal"channel.publish
approach.┆Issue is synchronized with this Jira Task by Unito
The text was updated successfully, but these errors were encountered: