Skip to content
This repository was archived by the owner on Jan 9, 2023. It is now read-only.

Conversation

adipirro
Copy link
Contributor

Since the stream needs to be reused, make it bypass the normal end process and just keep on eating put requests.

Since the stream needs to be reused, make it bypass the normal end process and just keep on eating put requests.
@coveralls
Copy link

coveralls commented Sep 18, 2018

Coverage Status

Coverage decreased (-0.006%) to 90.547% when pulling df1b302 on adipirro:patch-1 into c65140c on Unity-Technologies:master.

@stephen-palmer
Copy link
Contributor

hmm .. think I would feel better about just changing _onPut() to create a new NullStream each time.

In fact we could just simplify it further and do this inline:

this._putStream = new Writable({
  write(chunk, encoding, cb) {
    setImmediate(cb);
  }
});

Remove the NullStream class and _nullStream member var.

@adipirro
Copy link
Contributor Author

That should work if you'd prefer it. It is how I initially tested the fix.

Since the null stream never changes however...I felt this way was a little nicer. All we need is a black hole to dump things in, no need to constantly allocate new null streams every time a put request is blacklisted.

@stephen-palmer
Copy link
Contributor

I understand, i just think it's an over-optimization in this case. The normal mode of behavior would be to create a new stream for every PUT. This follow up PR is presumably required because there were side effects to the original cached null stream, which I think the proposed change will fix - but streams are sufficiently opaque/complex that I can't say for sure we're not missing anything.

Better to avoid the possibility all together by using new streams for each request, and it reduces overall complexity.

@stephen-palmer
Copy link
Contributor

Thanks, looks good.

@stephen-palmer stephen-palmer merged commit d710bf5 into Unity-Technologies:master Sep 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants