Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] capture: do not close tmpfile buffer, but redirect #6034

Closed
wants to merge 8 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Oct 22, 2019

Fixes #5502

TODO:

@blueyed blueyed added type: bug problem that needs to be addressed plugin: capture related to the capture builtin plugin labels Oct 22, 2019
@blueyed blueyed force-pushed the fix-capture-close-atexit branch from df4e3a6 to b9d7d91 Compare October 23, 2019 01:31
@blueyed blueyed force-pushed the fix-capture-close-atexit branch from 3e38537 to 5042501 Compare October 23, 2019 02:35
@blueyed blueyed closed this Oct 23, 2019
@blueyed blueyed deleted the fix-capture-close-atexit branch October 23, 2019 20:51
@blueyed blueyed restored the fix-capture-close-atexit branch October 23, 2019 21:08
@blueyed blueyed reopened this Oct 23, 2019
@blueyed blueyed force-pushed the fix-capture-close-atexit branch from 5042501 to 05405bb Compare October 23, 2019 21:12
@RonnyPfannschmidt
Copy link
Member

this seems icky to me, imho we shouldnt make pytest internals that much worse to work around other peoples broken setups

@blueyed
Copy link
Contributor Author

blueyed commented Oct 24, 2019

we shouldnt make pytest internals that much worse to work around other peoples broken setups

It is not really much worse, and has e.g. the benefit to re-use tmpfiles, which seems like a good idea in general to me.

@RonnyPfannschmidt
Copy link
Member

upon closer reading its indeed not that bad, but it gets trickier and trickier to follow

i believe by taking in py.io.capture we might be able to simplify it overall + perhaps even enable stdio output as views into the files to safe ram on larger test-suites as follow up

@blueyed
Copy link
Contributor Author

blueyed commented Oct 24, 2019

i believe by taking in py.io.capture

I think that's not wanted in general. And it appears to not have any enhancements? (IIRC it looks like pytest adopted it)

@blueyed blueyed force-pushed the fix-capture-close-atexit branch from 5f1864d to 7deb6d2 Compare November 2, 2019 12:35
@nicoddemus
Copy link
Member

upon closer reading its indeed not that bad, but it gets trickier and trickier to follow

I agree at first glance seems complicated, but I have some suggestions which I believe can improve the code and leave it more readable (I suspect this is just a quick first version to see the reception and if it would work).

@@ -77,19 +81,29 @@ def __init__(self, method):
self._method = method
self._global_capturing = None
self._current_item = None
self._atexit_funcs = [] # type: List[Callable]
atexit.register(self._atexit_run)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tmpfile management can be isolated in a separate class, which would greatly simplify the code:

class Streams(IntEnum):

    STDIN = 0
    STDOUT = 1
    STDERR = 2


@attr.s
class TemporaryFilePool:

    _tmpfiles = attr.ib(default=attr.factory(dict))

    def obtain_tmpfile(self, stream_id):
        try:
            tmpfile = self._tmpfiles[stream_id]            
        except KeyError:
            if stream_id == Streams.STDIN:
                tmpfile = open(os.devnull, "r")
            else:                
                f = TemporaryFile()
                with f:
                    tmpfile = safe_text_dupfile(f, mode="wb+")
            self._tmpfiles[stream_id] = tmpfile
        
        assert not tmpfile.closed
        return tmpfile

    def close(self):
        for f in self._tmpfiles.values():
            f.close()
        self._tmpfiles.clear()

We should then make CaptureManager create and maintain the tmpfile_pool, and pass it around to FDCaptureBinary. CaptureManager should then register the atexit handle itself, so these 3 lines would become:

self._tmpfile_pool = TemporaryFilePool()
atexit.register(self._tmpfile_pool)

This design also allows us to change the pool implementation, so we could even possibly control it via a config variable and get back the old behavior (I'm not actually suggesting it, just mentioning that this is then possible).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I also realize now that we don't respect basetemp when creating TemporaryFiles...

def _getcapture(self, method):
if method == "fd":
return MultiCapture(out=True, err=True, Capture=FDCapture)
return MultiCapture(out=True, err=True, Capture=FDCapture, capman=self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we would pass self._tmpfile_pool to the MultiCapture objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think passing capman is more universal.
(IIRC I am using this when auto-suspending on readin from stdin)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more universal means more coupling here... I prefer to pass only the necessary. I also suggest making it mandatory, so we don't have to check for its existense first (might require a few test changes I suppose).

@@ -551,15 +573,33 @@ def __init__(self, targetfd, tmpfile=None):
self.done = self._done
if targetfd == 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this entire if/else can now be just:

tmpfile = self._tmpfile_pool.obtain_tmpfile(self.targetfd)

@nicoddemus
Copy link
Member

I also wonder if we should force that all handles are closed with testdir?

@blueyed blueyed changed the title capture: do not close tmpfile buffer, but redirect [WIP] capture: do not close tmpfile buffer, but redirect Nov 6, 2019
@blueyed
Copy link
Contributor Author

blueyed commented Nov 6, 2019

@nicoddemus
Thanks for the review/suggestions.
Looking forward to implement them soon.
btw: I've experimented with having stdout and stderr duplicated to a third output, so that they can be displayed in order (which is useful e.g. when dropping into pdb). Should be considered to be supported here likely.

@blueyed
Copy link
Contributor Author

blueyed commented Nov 6, 2019

I also wonder if we should force that all handles are closed with testdir?

What do you mean?
Ensuring that every test gets new ones?

@nicoddemus
Copy link
Member

btw: I've experimented with having stdout and stderr duplicated to a third output, so that they can be displayed in order (which is useful e.g. when dropping into pdb).]

I like the idea of displaying them order, others are not so keen. But to clarify, do you mean create a new stream (stdouterr) in addition to stdout/stderr capture, or replacing them?

@blueyed
Copy link
Contributor Author

blueyed commented Nov 7, 2019

I like the idea of displaying them order, others are not so keen.

Haven't heard other opinions.

But to clarify, do you mean create a new stream (stdouterr) in addition to stdout/stderr capture, or replacing them?

It would be a seperate one.
I'd like to use it for e.g. "suspend-on-stdin".
In general I could imagine to having a setting which would allow to have "captured output" instead of "captured stdout" and "captured stderr". But with all three streams it might still be useful to show/handle stderr explicitly.

@nicoddemus
Copy link
Member

nicoddemus commented Nov 7, 2019

Haven't heard other opinions.

I haven't heard much against it either, except from a colleague at work (which is a great developer).

It would be a seperate one.
I'd like to use it for e.g. "suspend-on-stdin".

Hmm not sure what you mean here, can you please clarify?

Anyway I really like the "captured output" idea, but this seems like the topic for a separate PR/issue.

What do you mean?
Ensuring that every test gets new ones?

Yes, but I was just wondering, not sure if this is a good idea or not. 😁

@blueyed blueyed force-pushed the fix-capture-close-atexit branch from 7deb6d2 to 06f2154 Compare November 19, 2019 16:52
@blueyed blueyed closed this Nov 27, 2019
@sweenu
Copy link

sweenu commented Nov 27, 2019

Is this abandoned?

@wanam
Copy link

wanam commented Dec 3, 2019

@blueyed Any reason why you closed this merge request?

@blueyed
Copy link
Contributor Author

blueyed commented Dec 3, 2019

@sweenu @wanam
I'm not planning to work on this during the next time here.
Feel free to pick it up yourself already, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: capture related to the capture builtin plugin type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants