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

publish method on the Realtime (and REST) client object #64

Open
mattheworiordan opened this issue Jul 20, 2018 · 9 comments
Open

publish method on the Realtime (and REST) client object #64

mattheworiordan opened this issue Jul 20, 2018 · 9 comments

Comments

@mattheworiordan
Copy link
Member

mattheworiordan commented Jul 20, 2018

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:

client.channels.get('foo').publish(name, data);
client.channels.get('bar').publish(name, data);

Unless I am mistaken, that approach will result in channels foo and bar 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:

client.publish(channel, name, data, extras)

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

@JoHuang
Copy link

JoHuang commented Jul 25, 2018

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.

@mattheworiordan
Copy link
Member Author

Hi, I think the wasted memory allocation should be released if it became inactive after some time.

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.

Or right now it's an issue to us that the memory keeps increasing just like memory leak on our node.js server.

So why not release the channels you no longer need explicitly?

@mattheworiordan
Copy link
Member Author

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.

@mattheworiordan
Copy link
Member Author

@QuintinWillison @paddybyers @SimonWoolf any further thoughts about this going into the 1.2 spec or not?

@QuintinWillison
Copy link
Contributor

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.

@paddybyers
Copy link
Member

In the case of both REST and Realtime this feels like a client-side convenience wrapper around existing functionality.

Yes, that's correct.

@mattheworiordan
Copy link
Member Author

In the case of both REST and Realtime this feels like a client-side convenience wrapper around existing functionality.

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.

@mattheworiordan
Copy link
Member Author

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.

screenshot_2020-09-01_07-27-52_am

@QuintinWillison QuintinWillison transferred this issue from ably/docs Oct 3, 2022
@sync-by-unito
Copy link

sync-by-unito bot commented Oct 17, 2022

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-2809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants