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

"flush" event should always fire after "flush" call #446

Merged
merged 4 commits into from
Sep 15, 2014
Merged

"flush" event should always fire after "flush" call #446

merged 4 commits into from
Sep 15, 2014

Conversation

compulim
Copy link
Contributor

If nothing to flush, there will be no "flush" event from native stream.
Thus, the "open" event will never be fired, see _createStream.createAndFlush function, line 436 of file.js.
That means, self.opening will never set to false and no logs will be ever written to disk.

To prevent this from happening, we will always fire "flush" event despite there are anything to flush or not.

@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 11, 2014

@compulim these all sound reasonable (talking of your other PR as well). Having not run into specific issues here, could you perhaps create a test case that previously failed without these? That would be super helpful for preventing regressions.

@compulim
Copy link
Contributor Author

Will include commits from #447.

@jcrugzz I have implemented tests. But since it's about racing conditions between flush() and drain events. I cannot guarantee the test would always fail. In my dev box (i7), it fail about 20% of the time. But on my prod box (Azure A0), it fails almost 100% of the time.

Is it okay to submit such tests?

@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 11, 2014

@compulim that sounds fine. seems like it has to do with azure having a slower filesystem where this is more pronounced.

@compulim
Copy link
Contributor Author

I have included two tests for the fix. Thanks!

jcrugzz added a commit that referenced this pull request Sep 15, 2014
"flush" event should always fire after "flush" call
@jcrugzz jcrugzz merged commit f7d401b into winstonjs:master Sep 15, 2014
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.

3 participants