1708643 - Add and use QM_OR_ELSE_WARN_IF/QM_OR_ELSE_NOTE_IF/QM_OR_ELSE_LOG_IF macros

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task, P2)

Description

[Jan Varga [:janv]]

In case the orElseFunc just propagates some kinds of errors, we output both a warning and then an error. This will cause a split in the stacks during processing despite the fact that errors are propagated.

The original patch for the warning macros has this comment:

// XXX In case the orElseFunc just propagates some kinds of errors, we will
// output both a warning and (then) an error. This is not too bad, but maybe
// also not the nicest thing. However, addressing this would require two steps:
// first determining whether an error of expr is going to be handled in some
// way, and then possibly passing it to that handler.

That is probably quite hard to implement, because the goal there is to report either one WARNING or one ERROR (if I understood it correctly).

However it would be quite easy to check the result of orElseFunc and either report an error or warning depending on new result produced by orElseFunc.

So, it would work like this:

1. if there is no failure, QM_OR_ELSE just moves the success over.
2. If there's a failure and orElseFunc returns Ok, a warning is reported and no error is reported by the enclosing QM_TRY
3. If there's a failure and orElseFunc propagates the error, an error is reported and then another error is reported by the enclosing QM_TRY

This is still not ideal solution because instead of reporting just one WARNING or ERROR, we will report two ERRORs, but that is only a cosmetic issue (one extra ERROR event).
Actually, it quite makes sense because the error is "re-thrown" in orElseFunc

And we probably need a new name for the macro after these changes.

Comment 1

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #0)

However it would be quite easy to check the result of orElseFunc and either report an error or warning depending on new result produced by orElseFunc.

So, it would work like this:

1. if there is no failure, QM_OR_ELSE just moves the success over.
2. If there's a failure and orElseFunc returns Ok, a warning is reported and no error is reported by the enclosing QM_TRY
3. If there's a failure and orElseFunc propagates the error, an error is reported and then another error is reported by the enclosing QM_TRY

This is still not ideal solution because instead of reporting just one WARNING or ERROR, we will report two ERRORs, but that is only a cosmetic issue (one extra ERROR event).
Actually, it quite makes sense because the error is "re-thrown" in orElseFunc

And we probably need a new name for the macro after these changes.

Checking the result of orElseFunc is not optimal either because it would cause wrong ordering of QM_TRY events.

I figured out something else. We basically need a new method Result::orElseIf and then QM_OR_ELSE_WARN_IF macro.
The idea is that orElseIf first checks if the error code should cause a fallback (using a predicate), if not, the error is just propagated.

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1708643 - Use a common internal macro for definition of QM_OR_ELSE_(WARN|NOTE|LOG); r=#dom-storage

Depends on D114739

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1708643 - Rename orElseFunc argument to fallback in QM_OR_ELSE_REPORT definition; r=#dom-storage

Depends on D114842

Comment 4

[Jan Varga [:janv]]

Attachment: Bug 1708643 - Add QM_OR_ELSE_WARN_IF/QM_OR_ELSE_NOTE_IF/QM_OR_ELSE_LOG_IF macros; r=#dom-storage

Depends on D114843

Comment 5

[Jan Varga [:janv]]

Attachment: Bug 1708643 - QM: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=#dom-storage

Depends on D114844

Comment 6

[Jan Varga [:janv]]

Attachment: Bug 1708643 - Fix the comment for QM_OR_ELSE_WARN; r=#dom-storage

Depends on D114843

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1708643 - Add generic predicate and fallback functions for QM_OR_ELSE_(WARN|NOTE|LOG)_IF; r=#dom-storage

Depends on D114844

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1708643 - Add QM_OR_ELSE_LOG and QM_OR_ELSE_LOG_IF macros; r=#dom-storage

Depends on D114846

Comment 9

[Phabricator Automation]

Comment on attachment 9221510 [details]
Bug 1708643 - Add QM_OR_ELSE_LOG and QM_OR_ELSE_LOG_IF macros; r=#dom-storage

Revision D114936 was moved to bug 1701346. Setting attachment 9221510 [details] to obsolete.

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1708643 - IDB: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=#dom-storage

Depends on D114846

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1708643 - CACHE: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=#dom-storage

Depends on D115131

Comment 12

[Jan Varga [:janv]]

Attachment: Bug 1708643 - CACHE: Replace QM_TRY(QM_OR_ELSE_WARN(...)) with QM_WARNONLY_TRY(...) in MaybeUpdatePaddingFileInternal; r=#dom-storage

Depends on D115150

Comment 13

[Jan Varga [:janv]]

Attachment: Bug 1708643 - CACHE: Replace QM_TRY(QM_OR_ELSE_WARN(...)) with QM_TRY and a cleanup function in CreateOrMigrateSchema; r=#dom-storage

Depends on D115151

Comment 14

[Jan Varga [:janv]]

Attachment: Bug 1708643 - LS: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=#dom-storage

Depends on D115152

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f33d5c6ceb47
Use a common internal macro for definition of QM_OR_ELSE_(WARN|NOTE|LOG); r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/01be70418532
Rename orElseFunc argument to fallback in QM_OR_ELSE_REPORT definition; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/16915d90a511
Fix the comment for QM_OR_ELSE_WARN; r=dom-storage-reviewers,jstutte

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f33d5c6ceb47
https://hg.mozilla.org/mozilla-central/rev/01be70418532
https://hg.mozilla.org/mozilla-central/rev/16915d90a511

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cf5ba27193e
Add QM_OR_ELSE_WARN_IF/QM_OR_ELSE_NOTE_IF/QM_OR_ELSE_LOG_IF macros; r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/007ed77b9cf0
Add generic predicate and fallback functions for QM_OR_ELSE_(WARN|NOTE|LOG)_IF; r=dom-storage-reviewers,asuth

Comment 18

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/6cf5ba27193e
https://hg.mozilla.org/mozilla-central/rev/007ed77b9cf0

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b6fea1b0638
QM: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/2962c68b44e5
IDB: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/a2ef523fd8b4
CACHE: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/fa200b5fb647
CACHE: Replace QM_TRY(QM_OR_ELSE_WARN(...)) with QM_WARNONLY_TRY(...) in MaybeUpdatePaddingFileInternal; r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/cb1824657a65
CACHE: Replace QM_TRY(QM_OR_ELSE_WARN(...)) with QM_TRY and a cleanup function in CreateOrMigrateSchema; r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/8ac77d476c0e
LS: Replace QM_OR_ELSE_(WARN|LOG) with QM_OR_ELSE_(WARN|LOG)_IF in places where the fallback needs to be called conditionally; r=dom-storage-reviewers,asuth

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/3b6fea1b0638
https://hg.mozilla.org/mozilla-central/rev/2962c68b44e5
https://hg.mozilla.org/mozilla-central/rev/a2ef523fd8b4
https://hg.mozilla.org/mozilla-central/rev/fa200b5fb647
https://hg.mozilla.org/mozilla-central/rev/cb1824657a65
https://hg.mozilla.org/mozilla-central/rev/8ac77d476c0e