mach should handle a corrupted warnings database

RESOLVED FIXED in mozilla21

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: Zuhao(Joe) Chen)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mach])

Attachments

(1 attachment, 2 obsolete attachments)

During a build, the filesystem ran out of free space. Mach died when writing the compilation database. No big deal there.

The problem was the next time I ran |mach build|:

Error running mach:

    ['build']

The error occured in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

ValueError: Unterminated string starting at: line 6279 column 22 (char 266212)

  File "/home/gps/src/build-system/python/mozbuild/mozbuild/mach_commands.py", line 35, in build
    warnings_database.load_from_file(warnings_path)
  File "/home/gps/src/build-system/python/mozbuild/mozbuild/compilation/warnings.py", line 264, in load_from_file
    self.deserialize(fh)
  File "/home/gps/src/build-system/python/mozbuild/mozbuild/compilation/warnings.py", line 243, in deserialize
    obj = json.load(fh)
  File "/usr/lib/python2.7/json/__init__.py", line 280, in load
    **kw)
  File "/usr/lib/python2.7/json/__init__.py", line 328, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 365, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 381, in raw_decode
    obj, end = self.scan_once(s, idx)

IMO we should automatically replace corrupted on unreadable databases. Perhaps we spit out a warning in the process.
(Assignee)

Comment 1

5 years ago
Created attachment 705242 [details] [diff] [review]
A quick fix try to fix this bug.

A quick fix try to fix this bug by adding a verification method before reading database file into json.
Attachment #705242 - Flags: review?(gps)
Comment on attachment 705242 [details] [diff] [review]
A quick fix try to fix this bug.

Review of attachment 705242 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the effort. However, I'd prefer that we avoid double loading of the file. I would either wrap load_from_file() in a try..except everywhere or I would modify load_from_file() to trap exceptions and return a boolean.

If you modify warnings.py, a unit test would also be nice. See the test_warnings.py file. You can execute all the tests by running from your objdir: |make -C python check|
Attachment #705242 - Flags: review?(gps) → feedback+
(Assignee)

Comment 3

5 years ago
(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 705242 [details] [diff] [review]
> A quick fix try to fix this bug.
> 
> Review of attachment 705242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the effort. However, I'd prefer that we avoid double loading
> of the file. I would either wrap load_from_file() in a try..except
> everywhere or I would modify load_from_file() to trap exceptions and return
> a boolean.
> 
> If you modify warnings.py, a unit test would also be nice. See the
> test_warnings.py file. You can execute all the tests by running from your
> objdir: |make -C python check|

Thank you for the feedback! You're right, it's kinda weird to load it twice. Let me try work around it.
(Assignee)

Comment 4

5 years ago
Created attachment 705469 [details] [diff] [review]
Rework after Gregory's feedback

Do I need a unit test for mach_command? It seems there is not test related to this module.
Attachment #705469 - Flags: review?(gps)
Comment on attachment 705469 [details] [diff] [review]
Rework after Gregory's feedback

Review of attachment 705469 [details] [diff] [review]:
-----------------------------------------------------------------

No, we don't need a unit test for the mach_commands.py file at this time.

I suppose this patch could be more thorough: we may see other types of exceptions and os.remove may fail. But, I'm not too worried about these. This bug was already an edge case. So, the implementation should be good enough. If we see additional failures, we'll deal with them.
Attachment #705469 - Flags: review?(gps) → review+
Assignee: nobody → joejoevictor
Status: NEW → ASSIGNED
Whiteboard: [mach][mentor=gps][lang=python] → [mach]
(Assignee)

Comment 6

5 years ago
Created attachment 705513 [details] [diff] [review]
bug 831168
Attachment #705242 - Attachment is obsolete: true
Attachment #705469 - Attachment is obsolete: true
Attachment #705513 - Flags: checkin?(gps)
I'm going to just set the whiteboard since I'm pretty pretty. Someone will magically come along and upload it within a few hours.
Whiteboard: [mach] → [mach][checkin-needed]
Attachment #705513 - Flags: checkin?(gps)
Status: ASSIGNED → NEW
Keywords: checkin-needed
Whiteboard: [mach][checkin-needed] → [mach]
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d54dfb62030
Keywords: checkin-needed
(In reply to Gregory Szorc [:gps] from comment #7)
> I'm going to just set the whiteboard since I'm pretty pretty.

Well you are a handsome man, but no need to inflate your own ego.
https://hg.mozilla.org/mozilla-central/rev/7d54dfb62030
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> (In reply to Gregory Szorc [:gps] from comment #7)
> > I'm going to just set the whiteboard since I'm pretty pretty.
> 
> Well you are a handsome man, but no need to inflate your own ego.

Sometimes I accidentally type words multiple multiple.
You need to log in before you can comment on or make changes to this bug.