Closed Bug 783054 Opened 7 years ago Closed 7 years ago

ping files that can't be read should be deleted

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(4 files, 1 obsolete file)

Currently they'll just sit around and sit around and we'll keep error on reading them.
Attached patch patch (obsolete) — Splinter Review
Attachment #652172 - Flags: review?(taras.mozilla)
Comment on attachment 652172 [details] [diff] [review]
patch

99% sure this .remove will fail on windows since the file is still open. Should add a testcase
Attachment #652172 - Flags: review?(taras.mozilla) → review-
Since the stream is closed, this _might_ work. However, indeed, a testcase should remove all doubt.
We're going to need the ability to write fake ping files in test_TelemetryPing.js; we might as well have a function for just doing it.
Attachment #652172 - Attachment is obsolete: true
Attachment #663408 - Flags: review?(taras.mozilla)
Closing streams appears to be idempotent, so making sure the stream is closed prior to deleting the file shouldn't cause any problems.
Attachment #663409 - Flags: review?(taras.mozilla)
This became much easier when I realized I didn't have to shoehorn it into the ping/response logic earlier in the file.
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #663410 - Flags: review?(taras.mozilla)
Attachment #663408 - Flags: review?(taras.mozilla) → review+
Comment on attachment 663409 [details] [diff] [review]
part 2 - delete ping files if there's an error whilst reading them

This is good. We should however record success/fail ratios here.
Attachment #663409 - Flags: review?(taras.mozilla) → review+
Comment on attachment 663410 [details] [diff] [review]
part 3 - add tests

This looks good, but as I mentioned above we should record success/failure in a histogram and check the histogram gets recorded correctly in the testcase. Please post that as a followup patch.
Attachment #663410 - Flags: review?(taras.mozilla) → review+
I can add more complete checking of the contents of the histogram if you like, but it didn't seem worth it.
Attachment #664569 - Flags: review?(taras.mozilla)
Attachment #664569 - Flags: review?(taras.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.