Closed Bug 1654295 Opened 4 years ago Closed 4 years ago

Refactor IOUtils error handling to improve consistency and reduce duplication

Categories

(Toolkit Graveyard :: OS.File, task)

Product:
Toolkit Graveyard
Component:
OS.File
Type:
task

Tracking

(firefox81 fixed)

Status:
RESOLVED FIXED
Milestone:
81 Branch
Tracking Flags:
Tracking Status
firefox81 --- fixed

People

(Reporter: mail, Assigned: mail)

References

Details

Attachments

(7 files)

Bug 1654295: Reject early when relative paths are used in IOUtils methods r?barret!,Gijs!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1654295: Refactor IOUtils::read to improve error handling r?barret!,Gijs!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1654295: Refactor IOUtils::writeAtomic to improve error handling r?barret!,Gijs!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1654295: Use Result as a return type where possible in IOUtils.cpp r?barret!,Gijs!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1654295: Reduce boilerplate in IOUtils success paths r?barret!,Gijs!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1654295: Reduce boilerplate in IOUtils failure paths r?barret!,Gijs!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1654295: Reduce boilerplate needed to run background I/O in IOUtils r?barret!
47 bytes, text/x-phabricator-request
Details | Review

Follow up from Gijs' comment on Bug 1653001.

IOUtils method implementations tend to use a lot of similar boilerplate. Refactor the implementation to improve consistency and code reusability while reducing boilerplate.

Assignee: nobody → krourke

This patch introduces a new REJECT_IF_RELATIVE_PATH macro for use in
IOUtils. Its usage ensures that every method rejects in the same way when
unsupported relative paths are passed as parameters to IOUtils public
methods.

This patch fully encapsulates the behaviour of the IOUtils::read method behind
the private IOUtils::ReadSync method. Error handling for these methods has
been updated to use the Result monad, which is favourable over a bare
nsresult.

This lays some of the ground work to further improve error handling in
IOUtils.

Depends on D84731

This patch introduces a new private IOUtils::WriteAtomicSync method, which
fully encapsulates the behaviour of IOUtils::writeAtomic. The private
IOUtils::WriteSync method has also been updated to use the Result monad as
a return type, which is favourable over a bare nsresult.

These changes together simplify the implementation of the public method, and
lay some of the ground work to further improve error handling in IOUtils.

Depends on D84732

This changes most private methods of IOUtils to use the Result monad as a
return type instead of a bare nsresult. This improves consistency of all of
the public method implementations, and will allow for the shared patterns to be
abstracted in a follow-up patch.

Depends on D84733

This change introduces some new methods to reduce repeated boilerplate around
promise usage in success paths of IOUtils method implementations.

Depends on D84734

This change completely overhauls the way errors are handled in IOUtils. With
the introduction of the new IOError type, a useful error message is paired
with an nsresult at the error site. This provides erroneous callers with more
information about what went wrong, while improving consistency with mapping
errors to DOMExceptions.

For error sites where it is not immediately clear what went wrong, messages can
be omitted. A default error message will be filled in corresponding with the
wrapped nsresult when the operation is rejected to the calling JavaScript.

Depends on D84735

Introduces a new RunOnBackgroundThread template method which abstracts away
most of the boilerplate needed to run sync I/O tasks with result reported by a
JS Promise on a background thread.

Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44191218e308
Reject early when relative paths are used in IOUtils methods r=barret,Gijs
https://hg.mozilla.org/integration/autoland/rev/8dd9ad718cf9
Refactor IOUtils::read to improve error handling r=barret,Gijs
https://hg.mozilla.org/integration/autoland/rev/c94e94a51e94
Refactor IOUtils::writeAtomic to improve error handling r=barret
https://hg.mozilla.org/integration/autoland/rev/6bac11641ad3
Use Result as a return type where possible in IOUtils.cpp r=barret,Gijs
https://hg.mozilla.org/integration/autoland/rev/eef5969a2e21
Reduce boilerplate in IOUtils success paths r=barret
https://hg.mozilla.org/integration/autoland/rev/08bc2a5eefd0
Reduce boilerplate in IOUtils failure paths r=barret
https://hg.mozilla.org/integration/autoland/rev/5759d0505c0e
Reduce boilerplate needed to run background I/O in IOUtils r=barret

The above failure appears to be due to an error where a format parameter was accidentally omitted. This is now fixed in this interdiff. Sorry for the trouble!

Flags: needinfo?(krourke)
Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f01e679dcf49
Reject early when relative paths are used in IOUtils methods r=barret,Gijs
https://hg.mozilla.org/integration/autoland/rev/856024752262
Refactor IOUtils::read to improve error handling r=barret,Gijs
https://hg.mozilla.org/integration/autoland/rev/b35628720fa6
Refactor IOUtils::writeAtomic to improve error handling r=barret
https://hg.mozilla.org/integration/autoland/rev/ef65a442f852
Use Result as a return type where possible in IOUtils.cpp r=barret,Gijs
https://hg.mozilla.org/integration/autoland/rev/ee16d284651e
Reduce boilerplate in IOUtils success paths r=barret
https://hg.mozilla.org/integration/autoland/rev/63c355562fa6
Reduce boilerplate in IOUtils failure paths r=barret
https://hg.mozilla.org/integration/autoland/rev/ea988e45584f
Reduce boilerplate needed to run background I/O in IOUtils r=barret
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: