Skip to content

Confusion with Reducer.Count(), memoization vs mutablity #453

Description

@mgravell

When using reducers, you might use (for example) Reducer.Count() to fetch the count. If you want to alias that to an explicit field, you might use reducer.As("my_alias"). Unfortunately:

  • Reducer.Count() is memoized and singleton, with a private .ctor
  • the aliasing via .As() treats it as mutable
  • there is also a .Alias with a setter

This means that competing uses smash into each-other, and randomly change the alias of unrelated queries, for example:

// query one
search.GroupBy(..., Reducers.Count().As("qty"))...

// unrelated, query two, could be on a different thread
unrelatedSearch.GroupBy(..., Reducers.Count().As("count"))...

Who gets qty and who gets count is effectively random at this point.

Related code:

internal static readonly Reducer Instance = new CountReducer();

This could even break arg composition in niche race cases when changing between aliased and non-aliased.

Options:

  1. remove the memoization, so each .Count() returns a different instance
  2. remove the assumption of safe mutability, so that:
  • .As(...) returns a different instance when the value changes
  • the .Alias setter throws an exception (we can also mask the setter via property overloading)

Option 1 is simpler and risk free, but has the negative side-effect is performance (more allocations)
Option 2 is more complex and risks runtime breaks

However, if people are using aliasing (and they probably should be), option 2 would end up with the same allocations anyway, so: my vote goes "keep it simple, option 1".

Any thoughts @uglide @atakavci ?

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions