This is an archive of the discontinued Mercurial Phabricator instance.

blackbox: do not cache file objects
ClosedPublic

Authored by quark on Sep 7 2017, 1:32 AM.

Details

Summary

Having the blackbox file objects cached in ui._bbfp could in theory be
troublesome if multiple processes (ex. chg servers) have file objects
referring to a same file. (Although I spent some time and failed to build a
convincing test case)

This patch makes blackbox re-open the file every time to make the situation
better. Ideally we also need proper locking.

The caching logic traces back to the commit introducing blackbox
(18242716a). That commit does not have details about why caching is
necessary. Consider the fact that blackbox logs are not many, it seems fine
to remove the fp cache to be more confident.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Sep 7 2017, 1:32 AM
quark edited the summary of this revision. (Show Details)Sep 7 2017, 2:04 AM
quark retitled this revision from blackbox: do not cache file handler to blackbox: do not cache file objects.
durham accepted this revision.Sep 7 2017, 8:18 PM
durham added a subscriber: durham.
durham added inline comments.
hgext/blackbox.py
188

Might be worth mentioning this behavior change in the commit message.

yuja accepted this revision.Sep 12 2017, 10:02 AM
yuja added a subscriber: yuja.

Having the blackbox file objects cached in ui._bbfp could in theory be
troublesome if multiple processes (ex. chg servers) have file objects
referring to a same file. (Although I spent some time and failed to build a
convincing test case)

Perhaps it was okay because fseek(SEEK_END) was implied by the 'a' mode,
and we always called fwrite() followed by fflush().

But I like this change so queued, thanks.

This revision is now accepted and ready to land.Sep 12 2017, 10:02 AM
This revision was automatically updated to reflect the committed changes.