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: premature channels state consumption in ChannelManager #1466

Merged
merged 7 commits into from
Feb 13, 2025

Conversation

isekovanic
Copy link
Contributor

CLA

  • I have signed the Stream CLA (required).
  • Code changes are tested

Description of the changes, What, Why and How?

There was an unfortunate bug with the pagination logic in the new ChannelManager that somehow evaded me in the last PR. The offset was not being calculated properly, particularly in cases where:

  • We might have state.channels receive updates from something other than pagination (WS events for instance) and it will then contain either broken pagination or duplicate values when actually paginating (for example if a WS event arrives as the query is being executed)
  • The state might also change since there's nothing from preventing it (setChannels is public)
  • Any other case really between 2 loadNext() invocations

For this purpose, when running the queries we always calculate the offset through state.channels.length and we make sure to run state.getLatestValue() as late as possible, particularly when there are HTTP calls involved (we can ignore the other instances of this happening).

In addition to fixing loadNext(), the relevant WS event handlers have also been fixed (ones that run asynchronously) to make sure the most up to date state is always there.

Changelog

Copy link
Contributor

github-actions bot commented Feb 12, 2025

Size Change: +884 B (+0.17%)

Total Size: 524 kB

Filename Size Change
dist/browser.es.js 115 kB +200 B (+0.17%)
dist/browser.full-bundle.min.js 62.7 kB +90 B (+0.14%)
dist/browser.js 116 kB +196 B (+0.17%)
dist/index.es.js 115 kB +203 B (+0.18%)
dist/index.js 116 kB +195 B (+0.17%)

compressed-size-action

@isekovanic
Copy link
Contributor Author

isekovanic commented Feb 12, 2025

This also makes it evident that offset is pretty much never going to be correct as state.channels gets populated from various WS events. In my eyes, we have 2 options:

  • We ignore this and only really use it as a reference pointer to what the last offset was since we anyway calculate the correct one before each query
  • We can add an update to offset on every setChannels invocation (a simple size calculation) and then have it be a lot more reliable
    • This naturally won't shield us from all such instances (for example, state.partialNext() or state.next() are very much still a thing) but it should make things more reliable

Thoughts ?

}, obj);

// works exactly the same as lodash.uniqBy
export const uniqBy = <T>(array: T[] | unknown, iteratee: ((item: T) => unknown) | keyof T): T[] => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the same implementation as in lodash obviously and it does not use their fast SetCache, but close enough for such small lists. Still ~ O(n).

src/channel_manager.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
@isekovanic isekovanic merged commit ab24bf8 into master Feb 13, 2025
5 checks passed
@isekovanic isekovanic deleted the fix/premature-channels-state-consumptinl branch February 13, 2025 10:58
@github-actions github-actions bot mentioned this pull request Feb 13, 2025
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.

4 participants