Issue6333
Created on 2009-06-24 01:42 by srid, last changed 2009-07-10 11:59 by hpk. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| issue6333repro.py | srid, 2009-07-08 22:34 | Repro script | ||
| Messages (16) | |||
|---|---|---|---|
| msg89653 - (view) | Author: Sridhar Ratnakumar (srid) | Date: 2009-06-24 01:42 | |
The logging module has a bug that tries to call `flush' on a closed file handle (sys.std[out|err] to be specific). This bug was introduced by ConsoleHandler as defined in http://code.activestate.com/ recipes/576819/ The fix is simple: change definition of StreamHandler.flush in logging/ __init__.py to: def flush(self): if self.stream and hasattr(self.stream, 'flush') and not self.stream.closed: logging.StreamHandler.flush() |
|||
| msg89654 - (view) | Author: Sridhar Ratnakumar (srid) | Date: 2009-06-24 01:46 | |
BTW, this only happens when running the tests via py.test - http:// pytest.org ... perhaps threading/multiprocess issue. |
|||
| msg90148 - (view) | Author: Vinay Sajip (vinay.sajip) * ![]() |
Date: 2009-07-05 09:32 | |
Can you submit a short script which demonstrates the bug? Your fix is unfortunately not suitable to use as-is, since the StreamHandler operates on file-like objects, and these are not guaranteed to have the "closed" attribute. |
|||
| msg90287 - (view) | Author: Sridhar Ratnakumar (srid) | Date: 2009-07-08 22:34 | |
I have attached a script that repros this bug. Run it using py.test
(instructions inside the file).
Workaround is by uncommenting the last few lines ('flush' method).
|
|||
| msg90288 - (view) | Author: Sridhar Ratnakumar (srid) | Date: 2009-07-08 22:39 | |
Note: It appears that py.test assigns a temporary file to sys.std[out| err]. |
|||
| msg90292 - (view) | Author: Vinay Sajip (vinay.sajip) * ![]() |
Date: 2009-07-08 23:06 | |
Perhaps it's a py.test issue. Have you tried with unittest and nose? Remember, py.test has had little love for a while and so perhaps has a few rougher edges (though I hear in the blogosphere that the project may be rejuvenated shortly). |
|||
| msg90294 - (view) | Author: Sridhar Ratnakumar (srid) | Date: 2009-07-08 23:19 | |
Perhaps there is an issue with py.test. My understanding is that: if the logging module is designed to let users implement their own custom handlers (like I did with ConsoleHandler), is it not expected that they may do whatever they want with it including closing the file (in this case - sys.stdout is a temporary file that is obviously closed at one stage)? Shouldn't the logging module be reliable in such cases? I will point holger (author of py.test) to this issue in case if he has any comments. Anyways, this is a very low-prio bug for me .. especially when there is a workaround. |
|||
| msg90322 - (view) | Author: Vinay Sajip (vinay.sajip) * ![]() |
Date: 2009-07-09 10:10 | |
> Shouldn't the logging module be reliable in such cases? It tries to be, but it's neither omnipotent nor omniscient. It doesn't expect streams which it's using to be closed out from under it. See my comments about your ConsoleHandler recipe(s) on issue6345. Sorry if it seems I'm trying to rain on your parade - I'm just calling it as I see it. |
|||
| msg90324 - (view) | Author: holger krekel (hpk) | Date: 2009-07-09 10:47 | |
I think the issue is unrelated to py.test - it just presents a use case as it wants to run 1000's of tests and capture stdout/stderr per each test function, cannot guess about a test's logging-usage yet wants to minimize memory/resource usage and close any temporary files it opens. Anyway, a standalone minimal example involving the issue is this: import logging import StringIO stream = StringIO.StringIO() logging.basicConfig(stream=stream) stream.close() # to free memory/release resources At exit logging's shutdown() will try to flush/close resulting in an exception. Is it a problem to have the logging module be a bit more defensive and only try a flush/close if the stream isn't already closed? |
|||
| msg90328 - (view) | Author: Vinay Sajip (vinay.sajip) * ![]() |
Date: 2009-07-09 13:48 | |
> Is it a problem to have the logging module be a bit more defensive and > only try a flush/close if the stream isn't already closed? Not particularly, but it would be better if the following approach were taken: import logging import StringIO stream = StringIO.StringIO() h = logging.StreamHandler(stream) r = logging.getLogger() r.addHandler(h) #Do your testing work r.removeHandler(h) h.close() # closes underlying stream, also removes logging refs to h It should be understood that if a stream is passed to a StreamHandler, the ownership of that stream is passed as well, so closing the handler rather than the underlying stream is the right thing to do. ISTM this is good practice in general and not exclusive to logging. |
|||
| msg90344 - (view) | Author: holger krekel (hpk) | Date: 2009-07-09 19:33 | |
To recap the use case: stdout is redirected during a test function run which might trigger arbitrary usage of logging-functionality. Not closing the temporary file would mean that there could be as many open files as there are test functions - or one needs to rely on garbage collection for closing the resource - this is generally considered bad practise. So I consider it best practise to do resource cleanup immediately and close the temp file. Note that the test tool *does not touch the logging module at all*, and it has no way to mandate the usage of the logging module like you suggest. |
|||
| msg90345 - (view) | Author: Sridhar Ratnakumar (srid) | Date: 2009-07-09 19:56 | |
On Thu, 09 Jul 2009 06:48:28 -0700, Vinay Sajip <report@bugs.python.org> wrote: > It should be understood that if a stream is passed to a StreamHandler, > the ownership of that stream is passed as well, so closing the handler > rather than the underlying stream is the right thing to do. ISTM this is > good practice in general and not exclusive to logging. However, sys.stdout|err can be assigned to some thing else (eg: py.test assigning it to an open file object) .. in which case it is legitimate to handle (close) such objects (handles) by who created it (py.test). |
|||
| msg90363 - (view) | Author: Vinay Sajip (vinay.sajip) * ![]() |
Date: 2009-07-09 23:43 | |
> However, sys.stdout|err can be assigned to some thing else > (eg: py.test assigning it to an open file object) .. in which case it > is legitimate to handle (close) such objects (handles) by who > created it (py.test). @Sridhar: I disagree. Creation is not ownership. Clearly ownership is *initially* with the creator, but when a stream is used to initialise a StreamHandler, ownership of the stream passes to the StreamHandler. @Holger: I am not asking that py.test be specially modified to cater for use of a logging module it knows nothing about. But your example showed logging calls; I indicated what changes were needed to that sample to interoperate with logging. So if that was a part of the test cases, then the design of test cases which use logging (and not py.test itself, if it doesn't invoke logging directly) needs to consider the interaction between each test case, logging and py.test. And it is not solely the responsibility of py.test to minimise resource usage during test runs; part of this is also the responsibility of the test case design, which surely is why test frameworks have the concepts of setUp and tearDown. Your example calls logging.basicConfig passing a stream, and the stream is then immediately closed. If the test case had involved threads which did logging, for example, how could you guarantee that a call on the stream (other than flush) would never happen after the call to close the stream? Would I then have to ensure not only that flush was defensively programmed, but write as well? It seems like a can of worms to me, because of the propensity of this kind of approach to mask logic errors. I'm not saying you never have to resort to defensive programming - just that errors shouldn't pass silently where this is avoidable. The design of StreamHandler is predicated on it owning the stream (for example, it takes care to serialise access to the stream with locks, to allow thread-safe operation). This would be pointless if other code could arbitrarily operate on the stream without worrying about thread-safety. I also pointed out numerous flaws in Sridhar's reasoning both in this issue and in issue6435 (not py.test related) and in my view, he needs to look more closely at the logging package and the included functionality, and if necessary the core's regression test module, test_logging.py, to see what sorts of issues crop up. For example, I pointed out in msg90148 that file-like objects are not guaranteed to have a "closed" attribute, to which he has not responded. I would appeal for more attention to detail. And just in case it may seem otherwise, I'll state explicitly that I am not trying to be difficult. Similar issues have arisen during the initial development of test_logging.py. I will update the documentation for StreamHandler, FileHandler etc. to highlight the issue of ownership of the stream, as perhaps it's not clear enough. Sorry if this post seems a little long-winded. |
|||
| msg90367 - (view) | Author: Sridhar Ratnakumar (srid) | Date: 2009-07-10 00:14 | |
On Thu, 09 Jul 2009 16:43:49 -0700, Vinay Sajip <report@bugs.python.org> wrote: > Vinay Sajip <vinay_sajip@yahoo.co.uk> added the comment: > >> However, sys.stdout|err can be assigned to some thing else >> (eg: py.test assigning it to an open file object) .. in which case it >> is legitimate to handle (close) such objects (handles) by who >> created it (py.test). > @Sridhar: I disagree. Creation is not ownership. Clearly ownership is > *initially* with the creator, but when a stream is used to initialise a > StreamHandler, ownership of the stream passes to the StreamHandler. Are you suggesting that the ownership of `sys.stderr` belongs to the logging module once logging.basicConfig (that initializes a StreamHandler with stderr) is called? That no other module/library is to close sys.stderr even though they created it (sys.__stderr__ being the backup)? StreamHandler can take ownership of an arbitrary stream (say, created by the caller) passed to it, but assuming ownership of a standard stream, that are free to be overridden by a library (such as py.test), is rather bizarre. > I pointed out in msg90148 > that file-like objects are not guaranteed to have a "closed" attribute, > to which he has not responded. If this bug is acknowledged as a problem, then coming up with a fix is a different issue. I don't know how to detect whether a stream is closed or not .. especially when you say that not all file-like objects have a 'closed' attribute (BTW, which doesn't?), but this is the only solution I know currently: ... # some streams, such as sys.stdout|err, cannot be owned by StreamHandler # they might have been closed by whoever customized it. closed = hasattr(self.stream, 'closed') and self.stream.closed if self.stream and hasattr(self.stream, 'flush') and not closed: self.stream.flush() ... -srid |
|||
| msg90379 - (view) | Author: Vinay Sajip (vinay.sajip) * ![]() |
Date: 2009-07-10 08:03 | |
> Are you suggesting that the ownership of `sys.stderr` belongs to the
> logging module once logging.basicConfig (that initializes a
> StreamHandler with stderr) is called?
> That no other module/library is to close sys.stderr even though they
> created it (sys.__stderr__ being the backup)?
> StreamHandler can take ownership of an arbitrary stream (say, created
> by the caller) passed to it, but assuming ownership of a
> standard stream, that are free to be overridden by a library (such as
> py.test), is rather bizarre.
Actually, it's not bizarre at all. Are you saying it's OK for multiple
threads in a program to write to sys.stderr, whenever they want, without
restriction, even if the end result is end-user visible output which is
gibberish (for example data from two logical streams interspersed
randomly)? Just because it's a "standard" stream?
Also, perhaps you haven't noticed that StreamHandler has no way of
knowing whether the stream it was passed was a standard stream or not.
Logging may make certain hasattr-type checks e.g. for presence of
"flush" or "close", but it can't expect to not have ownership of the
stream, even for a standard stream.
I advise you to do the following: refactor your test cases to include
the explicit handler-adding code in your setup (instead of basicConfig),
and the handler-removing and closing code in your teardown.
def setUp(self):
self.hdlr = logging.StreamHandler(stream)
someLogger.addHandler(h)
def tearDown(self):
someLogger.removeHandler(self.hdlr)
self.hdlr.close()
where I have given someLogger module scope in the above snippet, you may
of course make it a testcase attribute too. I've used a unittest-style
of setUp/tearDown, I assume py.test is similar.
No changes to py.test are required. What happens if you do this?
|
|||
| msg90389 - (view) | Author: holger krekel (hpk) | Date: 2009-07-10 11:59 | |
Actually py.test catches stdout separately for setup and for the test code. Moreover, functional or integration test code (py.test is not only for unittests) can easily trigger some implicit logging-module usage which cannot eaysily be factored into a testcase-setup method, i am afraid. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2009-07-10 11:59:22 | hpk | set | messages: + msg90389 |
| 2009-07-10 08:03:05 | vinay.sajip | set | messages: + msg90379 |
| 2009-07-10 08:01:29 | vinay.sajip | set | messages: - msg90378 |
| 2009-07-10 08:00:02 | vinay.sajip | set | messages: + msg90378 |
| 2009-07-10 00:14:35 | srid | set | messages: + msg90367 |
| 2009-07-09 23:43:45 | vinay.sajip | set | messages: + msg90363 |
| 2009-07-09 23:08:29 | vinay.sajip | set | messages: - msg90346 |
| 2009-07-09 19:58:46 | srid | set | messages: + msg90346 |
| 2009-07-09 19:56:32 | srid | set | messages: + msg90345 |
| 2009-07-09 19:33:45 | hpk | set | messages: + msg90344 |
| 2009-07-09 13:48:27 | vinay.sajip | set | messages: + msg90328 |
| 2009-07-09 10:47:45 | hpk | set | nosy:
+ hpk messages: + msg90324 |
| 2009-07-09 10:10:58 | vinay.sajip | set | status: open -> closed resolution: invalid messages: + msg90322 |
| 2009-07-08 23:19:07 | srid | set | messages: + msg90294 |
| 2009-07-08 23:06:42 | vinay.sajip | set | messages: + msg90292 |
| 2009-07-08 22:39:02 | srid | set | messages: + msg90288 |
| 2009-07-08 22:34:03 | srid | set | files:
+ issue6333repro.py messages: + msg90287 |
| 2009-07-07 09:28:53 | ezio.melotti | set | priority: normal status: pending -> open stage: test needed versions: + Python 2.7, - Python 2.6 |
| 2009-07-05 09:32:47 | vinay.sajip | set | status: open -> pending nosy: + vinay.sajip messages: + msg90148 assignee: vinay.sajip |
| 2009-06-24 01:46:11 | srid | set | messages: + msg89654 |
| 2009-06-24 01:42:46 | srid | create | |
