Fix stdio File.open / resetFile / detach / refcount

[dkgroot]

Since #6382 was merged running phobos unittests will cause this (on DFly):

generated/dragonflybsd/debug/64/unittest/test_runner std.experimental.logger.core
****** FAIL debug64 std.experimental.logger.core
std.exception.ErrnoException@std/stdio.d(566): Could not close pipe `' (File exists)
----------------
std/exception.d:515 _init [0x5fe7c9]
std/exception.d:436 _init [0xb32e0e]
std/stdio.d:566 _init [0xd46333]
std/stdio.d:519 _init [0xd46099]
std/stdio.d:503 _init [0xd4600f]
std/experimental/logger/filelogger.d:83 _init [0x1ffe180]
std/experimental/logger/filelogger.d:42 _init [0x1ffe023]
std/experimental/logger/core.d:1808 _init [0x1fba600]
gmake[1]: *** [posix.mak:381: unittest/std/experimental/logger/core.run] Error 1

Looking at the sources the resetFile function looks suspect:
Orig source:

        ....
        if (_p is null)
        {
           _p = cast(Impl*) enforce(malloc(Impl.sizeof), "Out of memory");
            detach();
        }
        {
            if (atomicOp!"-="(_p.refs, 1) == 0)
                closeHandles();
            else
                _p = cast(Impl*) enforce(malloc(Impl.sizeof), "Out of memory");
        }
        ....

Notice, there is no else between these two blocks, so we always decrement _p.refs after the initial allocation of _p, which does not make sense. Just adding 'else' inbetween these blocks is not going to help.

If this decrement does not make the refs go to zero, we malloc _p again (memory leak ?). In short, if we are not (re-)opening a previously opened file, this results in malloc/free/malloc. If there was a previously open file (ie: _p ! is null) and only a single reference (refs==1) then we close the previous file, but do not malloc _p again before using it (null dereference down the line ?). If there were more references, we decrement the refcount by one and then malloc _p a second time. None of this really makes a lot of sense (to me, but maybe i am just missing something).

Reusing the detach function instead of trying to re-implement seems like the right choice in this situation.

BTW: Would it have been possible to use std/typecons RefCounted instead of reimplementing it locally/partially ?

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkgroot! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + phobos#6484"

[dkgroot]

@klickverbot Thanks for adding @JackStouffer !

[dkgroot]

Side Question / Unrelated:
Should we also fix the error output having mismatched quotes (In a seperate pull request if need be) after 'Could not close pipe' :
std.exception.ErrnoException@std/stdio.d(566): Could not close pipe `' (File exists)

Or was the use of these 'mismatched' quotes intensional ? This quoting style is seen in multiple location in std/stdio.d.

[CyberShadow]

Or was the use of these 'mismatched' quotes intensional ?

That style of quoting is frequently used in some places, like GNU documentation, though I think it is now considered outdated... in any case, it's probably intentional.

[dkgroot]

@CyberShadow
I was talking about this part
`'
Which looks rather strange. I would expect something like ' ' or " " or ‘ ’ (The latter are curly quotes, which would be quite annoying to maintain, but at least they are a matched pair).

[CyberShadow]

Yes, I understood you correctly. See here: https://stackoverflow.com/questions/15390066/why-does-the-shell-output-often-use-mixed-characters-in-its-output and https://english.stackexchange.com/questions/17695/any-reference-on-the-usage-of-a-backtick-and-single-quotation-mark-like-this

[dkgroot]

@CyberShadow Thanks for the links, learned something :-) Still don't like it, but will have to live with them i guess. Funny how you can be around for 45 odd years and never having noticed this quotation style being used before, guess i wasn't paying attention :-)

[andralex]

For the record, @CyberShadow is right - when I wrote those quotes I just followed a style popular at the time.

[JackStouffer]

@dkgroot I changed the base branch to stable as to get this into a release sooner. Can you please git rebase origin/stable to fix the commits?

[JackStouffer]

No need for this now, as it will always be true. detach marks p as null.

[dkgroot]

Thanks for the review / update... Removed second _p is null check.

[JackStouffer]

Please squish your changes.