Linting breaks when using multiple linters on same file

[IjzerenHein]

Hi, I've been debugging as issue regarding linter-jscs, and came across a problem in either the core linter-code or linter-jscs. I can't figure out how the code was intended to work, so I'm reporting it here.

The situation is that linter-jscs is calling LinterView.processMessage twice. This in turn causes the temporary file to be deleted before other linters are done processing it.
Linter.processMessage:

processMessage: (messages, tempFileInfo) =>
    tempFileInfo.completedLinters++
    @messages = @messages.concat(messages)
    if tempFileInfo.completedLinters == @linters.length
      fs.unlink tempFileInfo.path
    @display()

Now, looking the code, it wasn't clear to me at all, whether processMessage was allowed to run multiple times? Based on the name processMessage I would expect that it can be called multiple times, e.g. for every message that is outputted. However, looking at the implementation, it seems that it decreases the completedLinters counter for every-call. This seems like a bug to me, either in LinterView.processMessage or in `linter-jscs'.

The consequence is that the first linter seemingly works correctly. Even if it would call processMessage twice, the file would be in-use and can therefore not be unlinked. But this will cause the temporary file to not be cleaned up in the temp-directory. The second linter will experience problems like 'Cannot find file...',

A dime for your thoughts.

[dmnd]

Reading the code to linter, I don't think processMessage is supposed to be called more than once by the same linter. Ignoring the name, I can't see how it would be called more than once under normal circumstances. It looks to me like it's only called by the callback to lintFile, and that callback is only called once (unless I'm missing something).

Given that, it seems like the real fix should be to prevent whatever is causing processMessage to be called twice for the same linter. Have you identified why linter-jscs is calling processMessage twice?

[IjzerenHein]

No I don't know why it's been called twice. If you look at linter-jscs:
https://github.com/AtomLinter/linter-jscs/blob/master/lib/linter-jscs.coffee
, you will see it it a pretty straightforward linter implementation.
I've seen other linters call processMessage more than once as well, but the problem always went unnoticed when you are using only 1 linter.
Are you really really sure it is supposed to be called only once?

[dmnd]

I didn't write the original code so I don't know for certain, but I'm pretty sure. Reading the implementation, how do you think it's being called twice? It seems to me that's only possible if lintFile is called twice, or if errorStream is both stdout and stderr when the respective callbacks run.

If you have a solid repro you might be able to investigate by putting a console.trace() line to see what's triggering it.

[IjzerenHein]

Alright, I can't answer that right away, I'll try to create a repro and get back to you.
Cheers

[dmnd]

Thanks for spending time on this @IjzerenHein 👍

[dmnd]

Looks like @hd-deman merged #143 anyway - @hd-deman, do you have any thoughts on the above discussion?

[hd-deman]

Hm. multiple linters on same file work for me.

[dmnd]

Yeah, multiple linters in the same file works fine for me too. Still, there could be some kind of race condition causing this bug for other people.

What I wanted to ask you though @hd-deman is if processMessage is intended to be called more than once per linter instantiation. From reading the code I think the answer is no, but @IjzerenHein wasn't convinced.

[hd-deman]

the answer is no, it must called olny once per linter instantiation. so waiting for @IjzerenHein digging. I also digging deeper

[dmnd]

Cool. We can keep this issue open then. Maybe as a followup to #167 we should have the linter emit a warning if it detects that processMessage has been called multiple times for the same Linter instance.

[IjzerenHein]

If processMessage is really intended to be called once, then the problem must indeed be in the linter. In this case linter-jscs... I don't really have time to look at this problem right now, maybe next week.

[IjzerenHein]

Oh and if it's working on your system, that doesn't necessarily mean there is no bug there. If the linter already has a handle open to the file, then unlink doesn't work and then it all seems to work. Some linters are faster than others... You might want to put a breakpoint on the code and check whether ProcessMessage isn't really being called multiple times.

[dmnd]

@hd-deman & @IjzerenHein: Take a look at a462f36, I think that's what the issue was.

[IjzerenHein]

Yeah that looks like a great explanation & solution, excellent work @dmnd!

[hd-deman]

👍 tnx