off-by-one error causes Node to hang after progress is complete (0.1.0 regression) #19

Closed
glasser opened this Issue Nov 19, 2012 · 5 comments

4 participants

@glasser

This is a regression in 0.1.0 from 0.0.5.

Since progress now uses readline, it resumes stdin when you start your bar and tries to pause it again (via this.rl.close()) when the bar is complete. However, due to an off-by-one error, it doesn't actually call this.rl.close until 1 tick past total. So if you are using progress in a simple command-line tool which otherwise doesn't use stdin, using progress 0.1.0 means that stdin will be left resumed and your program will hang. For example, the following program:

var ProgressBar = require('progress');
var bar = new ProgressBar(':bar', {total: 2});
bar.tick();

hangs with 0.1.0 but not with 0.0.5.

The simplest fix for this would be to change the // progress complete code from

  if ((this.curr += len) > this.total) {

to

  if ((this.curr += len) >= this.total) {

However, it seems unfortunate that progress has to interact with stdin at all. Maybe just don't use readline? Or instead of passing process.stdin to readline, pass a dummy object that ignores the methods that are called on it?

I'm happy to submit a pull request to do any of the above, depending on which makes most sense to you.

@tj
Sloth member
tj commented Nov 21, 2012

meh doesn't matter to me I dont use this anymore, I can add you as a committer if you want

@tj
Sloth member
tj commented Nov 21, 2012

IMO fuck windows, it just messes with all simple things

@glasser

Uh, sure. Using readline was just for windows, so maybe we should just revert the readline change?
Or I'm happy to make the one-character > to >= change if you add me as a committer, or send you the pull request...

@tj tj added a commit that closed this issue Nov 21, 2012
@tj tj >=. Closes #19 008a2ec
@tj tj closed this in 008a2ec Nov 21, 2012
@Jabbers

lol I just ran into this issue.. perfect timing!

@piuccio

Would it be possible to publish a 0.1.1 release with this fix?

@tj tj added a commit that referenced this issue Jun 16, 2014
@tj tj >=. Closes #19 0ed2b1e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment