Filehandle leak on error #157

Merged
merged 2 commits into from Jul 13, 2012

Conversation

Projects
None yet
2 participants
@OrangeDog
Contributor

OrangeDog commented Jul 13, 2012

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 13, 2012

Why? That's implicit in JS, no?

Why? That's implicit in JS, no?

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 13, 2012

Owner

"function has inconsistent return points" was the warning

Owner

OrangeDog replied Jul 13, 2012

"function has inconsistent return points" was the warning

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 13, 2012

"I don't give a shit" would be my reply : p. (I see the point, I just don't agree with it).

"I don't give a shit" would be my reply : p. (I see the point, I just don't agree with it).

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 13, 2012

Owner

That's cool. I'll just do the line 14 fix and remove the rest from the Pull if you want.

Owner

OrangeDog replied Jul 13, 2012

That's cool. I'll just do the line 14 fix and remove the rest from the Pull if you want.

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 13, 2012

I like my trailing commas. They make it easier to edit and re-arrange things later.

I like my trailing commas. They make it easier to edit and re-arrange things later.

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 13, 2012

Owner

I like warning-free code. Feel free to put them back, I don't really mind - and it's my fault anyway for using a strict IDE.

Owner

OrangeDog replied Jul 13, 2012

I like warning-free code. Feel free to put them back, I don't really mind - and it's my fault anyway for using a strict IDE.

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 13, 2012

I also like warning free code. But I don't agree with the rules JSLint tries to enforce.

Either way, I merged it as I'd rather have the rest of your improvements (like the missing semicolons) as quick as possible : ).

I also like warning free code. But I don't agree with the rules JSLint tries to enforce.

Either way, I merged it as I'd rather have the rest of your improvements (like the missing semicolons) as quick as possible : ).

felixge added a commit that referenced this pull request Jul 13, 2012

@felixge felixge merged commit a3d7402 into felixge:master Jul 13, 2012

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 13, 2012

Owner

Thanks, commented on the lint stuff - but pulled it in anyway. But keep in mind if you send me patches again.

Owner

felixge commented Jul 13, 2012

Thanks, commented on the lint stuff - but pulled it in anyway. But keep in mind if you send me patches again.

@OrangeDog

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 26, 2012

Contributor

This needs another fix, but I can't work out how to do a pull request for just a single commit.
85e4209

Contributor

OrangeDog commented Jul 26, 2012

This needs another fix, but I can't work out how to do a pull request for just a single commit.
85e4209

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 26, 2012

Owner

@OrangeDog I was about to pull this in, but noticed a failing test probably related to your previous pull:

  /Users/felix/code/node-formidable/lib/incoming_form.js:259
    this.openedFiles.forEach(function(file) {
                     ^
  TypeError: Cannot call method 'forEach' of undefined
      at IncomingForm._error (/Users/felix/code/node-formidable/lib/incoming_form.js:259:20)
      at _error (/Users/felix/code/node-formidable/test/legacy/simple/test-incoming-form.js:553:8)
      at test (/Users/felix/code/node-formidable/test/legacy/simple/test-incoming-form.js:22:3)
      at Object.<anonymous> (/Users/felix/code/node-formidable/test/legacy/simple/test-incoming-form.js:544:1)
      at Module._compile (module.js:449:26)
      at Object.Module._extensions..js (module.js:467:10)
      at Module.load (module.js:356:32)
      at Function.Module._load (module.js:312:12)
      at Module.runMain (module.js:487:10)
      at process.startup.processNextTick.process._tickCallback (node.js:244:9)

Could you have a look? Then I'll pull the other patch and release a new npm version.

Owner

felixge commented Jul 26, 2012

@OrangeDog I was about to pull this in, but noticed a failing test probably related to your previous pull:

  /Users/felix/code/node-formidable/lib/incoming_form.js:259
    this.openedFiles.forEach(function(file) {
                     ^
  TypeError: Cannot call method 'forEach' of undefined
      at IncomingForm._error (/Users/felix/code/node-formidable/lib/incoming_form.js:259:20)
      at _error (/Users/felix/code/node-formidable/test/legacy/simple/test-incoming-form.js:553:8)
      at test (/Users/felix/code/node-formidable/test/legacy/simple/test-incoming-form.js:22:3)
      at Object.<anonymous> (/Users/felix/code/node-formidable/test/legacy/simple/test-incoming-form.js:544:1)
      at Module._compile (module.js:449:26)
      at Object.Module._extensions..js (module.js:467:10)
      at Module.load (module.js:356:32)
      at Function.Module._load (module.js:312:12)
      at Module.runMain (module.js:487:10)
      at process.startup.processNextTick.process._tickCallback (node.js:244:9)

Could you have a look? Then I'll pull the other patch and release a new npm version.

@OrangeDog

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 26, 2012

Contributor

Yep, that is what 85e4209 should fix.

The other submit on my branch is for #66, did you want me to sort out tests for that first or are you happy to take that for the next release as is?

Contributor

OrangeDog commented Jul 26, 2012

Yep, that is what 85e4209 should fix.

The other submit on my branch is for #66, did you want me to sort out tests for that first or are you happy to take that for the next release as is?

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 26, 2012

Owner

Yep, that is what 85e4209 should fix.

Nope, test still fails for me, even with that patch applied.

The other submit on my branch is for #66, did you want me to sort out tests for that first or are you happy to take that for the next release as is?

Seems like you closed #66, not sure what to do with it.

Owner

felixge commented Jul 26, 2012

Yep, that is what 85e4209 should fix.

Nope, test still fails for me, even with that patch applied.

The other submit on my branch is for #66, did you want me to sort out tests for that first or are you happy to take that for the next release as is?

Seems like you closed #66, not sure what to do with it.

@OrangeDog

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 26, 2012

Contributor

Nope, test still fails for me, even with that patch applied.

OK, see 258ed6a7cd3d25760b344c12836d47be897fde36.

Seems like you closed #66, not sure what to do with it.

No, it just got referenced from this "closed" pull request, but the issue is still open.
If you can give me a clue on how to do a pull request for only a subset of commits, I'll sort it out.

Contributor

OrangeDog commented Jul 26, 2012

Nope, test still fails for me, even with that patch applied.

OK, see 258ed6a7cd3d25760b344c12836d47be897fde36.

Seems like you closed #66, not sure what to do with it.

No, it just got referenced from this "closed" pull request, but the issue is still open.
If you can give me a clue on how to do a pull request for only a subset of commits, I'll sort it out.

@OrangeDog

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 26, 2012

Contributor

Scrap the commit in the middle - there's still a race condition in there where it will throw.

Contributor

OrangeDog commented Jul 26, 2012

Scrap the commit in the middle - there's still a race condition in there where it will throw.

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 30, 2012

Owner

Scrap the commit in the middle - there's still a race condition in there where it will throw.

Do you have a patch for this?

Owner

felixge commented Jul 30, 2012

Scrap the commit in the middle - there's still a race condition in there where it will throw.

Do you have a patch for this?

@OrangeDog

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 30, 2012

Contributor

Scrap the commit in the middle - there's still a race condition in there where it will throw.

Do you have a patch for this?

No. Will do a separate pull request for that whole thing once it's sorted.

Contributor

OrangeDog commented Jul 30, 2012

Scrap the commit in the middle - there's still a race condition in there where it will throw.

Do you have a patch for this?

No. Will do a separate pull request for that whole thing once it's sorted.

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge Jul 30, 2012

Owner

Sweet, thanks!

Owner

felixge commented Jul 30, 2012

Sweet, thanks!

@OrangeDog

This comment has been minimized.

Show comment
Hide comment
@OrangeDog

OrangeDog Jul 31, 2012

Contributor

To clarify, by "that whole thing" I mean #66, not the extra fixes to #157.

Contributor

OrangeDog commented Jul 31, 2012

To clarify, by "that whole thing" I mean #66, not the extra fixes to #157.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment