fix: report missing import properly in loadSync

[alexander-fenster]

Fixes #1954.

The finish function can be called in two modes, sync (when it throws an exception), or async (when it calls the callback). It makes sure the callback won't be called twice, but it makes one extra step and clean up the callback even it was called in sync mode. It causes problems such as in #1954 where instead of reporting "file not found" when parsing a proto, it jumps over that error and only reports the next error–or no error if the parsing happens to succeed without a bunch of imports being skipped as a result of the first exception.

Let's avoid clearing the callback if we haven't used it yet.

The problem actually occurs when loading a proto file in sync mode (e.g. by running root.loadSync(filename), which is what pbjs does), hence the PR title.

[bcoe]

I'm a little confused, is callback called elsewhere when using loadSync, rather than line 105?

[bcoe]

The style is internally consistent, so LGTM. But, it would be an improvement to use let / const to protect us from weird scope issues.

[alexander-fenster]

I'm a little confused, is callback called elsewhere when using loadSync, rather than line 105?

Only in line 105, but finish is called twice: one from the catch block on line 140, but before that, also from the fetch function called on line 133. The invocation from line 133 throws an exception that is caught by the catch block and is supposed to call the callback, but it was already set to null.