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

fix(channels): readd clearing and fix auto start #297

Merged
merged 21 commits into from
Jan 11, 2022

Conversation

samuelmasse
Copy link
Contributor

@samuelmasse samuelmasse commented Jan 10, 2022

This PR fixes some problems that were introduced by #270

Notably the notion of keeping a lru of most used channels, and clearing the ones that aren't used was removed. Since this is a potential memory leak (although you'd really need to have a lot of channels), this PR fixes that. It also fixes channels auto starting and bypassing the normal start for a channels. So for example a channel could start without producing a health event.

Since these changes concerned the instance service, I took the time to refactor it into multiple sub services since the code was getting messy

Closes DEV-2206
Closes DEV-2212

@linear
Copy link

linear bot commented Jan 10, 2022

DEV-2206 Add back clearing of channels

When extracting the channels in another package, the feature of removing the channels after a certain time that they are not used in the lru was removed so add it back

@samuelmasse
Copy link
Contributor Author

Note: It's possible that a channel gets cleared up while having messages queued to be sent out. In that case we should wait before clearing the channel, otherwise there will be very rare and very hard to debug misses when sending out messages.

@linear
Copy link

linear bot commented Jan 10, 2022

DEV-2212 Refactor instance service

Comes with DEV-2206

@samuelmasse samuelmasse marked this pull request as ready for review January 10, 2022 22:33
Copy link
Contributor

@laurentlp laurentlp left a comment

Choose a reason for hiding this comment

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

Nice work as usual!!

Here are some missing parts:

  • Autostart (botpress -> messaging) missing.
  • Autostart callback needs to handle the case where a conduit is errored and does not start it.

packages/channels/src/base/service.ts Show resolved Hide resolved
packages/channels/src/base/channel.ts Show resolved Hide resolved
@samuelmasse samuelmasse merged commit 8d01733 into master Jan 11, 2022
@samuelmasse samuelmasse deleted the sm-channel-clearing branch January 11, 2022 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants