Synchronous execution

[jpodwys]

Attempting to make .flush() execute synchronously--currently there is a race condition.

Moving this from #80 as requested.

[jpodwys]

This has all the commit history from #80 since it's a branch of that fork. When ready to merge, I can clean it up if desired.

[dougwilson]

This has all the commit history from #80 since it's a branch of that fork. When ready to merge, I can clean it up if desired.

I can squash it for you on merge, so it should not be an issue.

[dougwilson]

Thank you! So I have a couple issues here:

1. Can we figure out why exactly that Node.js 0.8 test is failing after this change?
2. The failing Node.js 0.8 test revealed a serious issue: calling res.flush() can cause an exception to occur that is impossible to catch. You probably need to catch that error in the code here and properly forward it as an event to the appropriate stream so a user can actually catch the error.
3. Ideally, we should have a test added that fails without this change and passes with the change, so we don't accidentlly break the proper flush behavior in the future. That should definitely be possible to achieve since you saw the issue in your own code somehow.

[jpodwys]

Thanks for the notes! I may not be able to get to them until some time on Thursday, but it shouldn't be later than that.

[jpodwys]

In my latest push, Travis CI lists all checks as passing.

Per your comments above:

1. It seems that, in 0.8, .end was still happening before at least one .write or .flush had executed. This push fixes that issue.
2. You say "catch that error in the code" and "forward it as an event to the appropriate stream." Unless you're simply referring to a try/catch, I think I need some additional direction here. Furthermore, when I originally submitted Adding callback to res.write and res.end for streaming support #80, you noticed there were really two issues and requested I separate them into two PRs. With that in mind, shouldn't fixing this bug that appears to have been here for a long time be part of yet another PR?
3. Are you saying I should write a test that toggles the synchronous execution I've added? If so, do you have any pointers for how I can do that without polluting compression's public API? I suppose the unit test could just temporarily overwrite a couple of compression's functions to toggle the behavior off.

[dougwilson]

Hi @jpodwys,

It seems that, in 0.8, .end was still happening before at least one .write or .flush had executed. This push fixes that issue.

Nice!

You say "catch that error in the code" and "forward it as an event to the appropriate stream." Unless you're simply referring to a try/catch , I think I need some additional direction here.

I don't have specific direction here until I have time to dig in, so wanted to describe it in words. If you want to implement it as try/catch, that should be fine.

Furthermore, when I originally submitted #80, you noticed there were really two issues and requested I separate them into two PRs. With that in mind, shouldn't fixing this bug that appears to have been here for a long time be part of yet another PR?

This is not a long-time bug, it's one you just created in this pull request by moving the stream.flush() call to a different tick than the res.flush() call. Prior to your changes here, users could (and do) try { res.flush() } and catch that thrown error from stream.flush(), but now, it's impossible to catch. Impossible-to-catch errors are a huge issue in Node.js code, so we cannot merge this without that addressed.

Are you saying I should write a test that toggles the synchronous execution I've added? If so, do you have any pointers for how I can do that without polluting compression 's public API? I suppose the unit test could just temporarily overwrite a couple of compression 's functions to toggle the behavior off.

I'm saying that you MUST have a test added to the suite that does not pass without the changes in index.js and passes with the changes, otherwise we will end up breaking this, and we don't want that. We maintain this module on a vonunteer basis, and as such, a test suite is arguably the most valuable part, to ensure that this module continues to work, even between maintainers.

I assumg you are fixing this bug because you saw external behavior that revealed it. I'm saying that you need to codify that behavior as a test here. Absoutely you are not to do anything you were not doing in your own code when you saw the bug. For example, if you were seeing this bug in your app, clearly you do not need any new public APIs here to reproduce the issue, since they didn't exist when you had the issue. You are not to overwrite any methods, either, unless you are saying that you were doing that in you application that had this issue?

I don't have any specific pointers, since you have never actually shared with me a full server that had the issue (only code snippets and a description of the issue), and I would have to take time away from dealing with a security issue release to look harder, so if I were to, it may be a while.

[jpodwys]

Thanks for the feedback! I'll have more for you later this week.