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

Scan/Min/Max transformer may has side effect leaking #74

Closed
lestathc opened this issue Feb 27, 2017 · 3 comments
Closed

Scan/Min/Max transformer may has side effect leaking #74

lestathc opened this issue Feb 27, 2017 · 3 comments

Comments

@lestathc
Copy link

Considering I have such code:

var transformer = new ScanStreamTransformer(myPredictFunc);
var scan1 = myStream.transform(transformer);
var scan2 = myAnotherStream.transform(transformer);

I may dirty the intermedia result of scan transformer.

Given the transforms are public, if I only want to reuse the transforms without using the Observable interface, and according to "It is good practice to write transformers that can be used multiple times.". I may want to use transformer like that.

How about either mark transformers private, or document them/add annotations to indicate that these transformers is not sharable.

@brianegan
Copy link
Collaborator

Hrm, good catch. I'd personally like to keep the transformers Public, as that allows folks to use all of our Stream and StreamTransformers classes individually without needing to pull in all of Observable if they really only need one or two items.

In this case, great call. We should inspect our operators and make sure they can be reused. Secondly, we defo need more docs on these classes. We've just made them public without much explanation thus far, something we'll certainly fix :)

@frankpepermans
Copy link
Member

hmmm, we could fix this by storing values in a Map for example,

take the min/max transformer, instead of storing a single value in the transformer class, we could create a Map<EventSink, T> instead, which stores values in relation to the provided Stream sink.

On close, we could delete the related EventSink key from that Map.

@brianegan
Copy link
Collaborator

brianegan commented Feb 27, 2017

Another option: Now that we've got a class with an internal factory method, every time bind is called, we could build a new transformer internally and bind that, rather than building the transformer immediately upon creation and re-binding that over and over.

Although I think we should generally review our transformers and see if we can avoid doing that, for some cases it might make sense to ensure we don't have state problems.

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

No branches or pull requests

3 participants