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

Added brackets to list comprehension, fixing this exception: #174

Merged
merged 2 commits into from Jan 17, 2019

Conversation

Projects
None yet
3 participants
@ricardopinto
Copy link
Contributor

ricardopinto commented Jul 6, 2018

Exception ignored in: <generator object _find_first_app_frame_and_name.<locals>.<genexpr> at 0x10693ad00>
Traceback (most recent call last):
  File "/Users/ricardopinto/a_project/env/lib/python3.6/site-packages/structlog/_frames.py", line 42, in <genexpr>
    while any(name.startswith(i) for i in ignores):
SystemError: error return without exception set

From what I saw, ignores was ['structlog', 'logging'], and name was structlog._frames.
Python version is 3.6.5, structlog is 18.1.0.

Added brackets to list comprehension, fixing this exception:
Traceback (most recent call last):
  File "/Users/ricardopinto/a_project/env/lib/python3.6/site-packages/structlog/_frames.py", line 42, in <genexpr>
    while any(name.startswith(i) for i in ignores):
SystemError: error return without exception set
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 6, 2018

Codecov Report

Merging #174 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #174   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines         822    822           
  Branches      106    106           
=====================================
  Hits          822    822
Impacted Files Coverage Δ
src/structlog/_frames.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd01af7...b312ab2. Read the comment docs.

@hynek

This comment has been minimized.

Copy link
Owner

hynek commented Jul 7, 2018

I’d need to see a breaking test for the old code.

IMHO the code itself should be fine: it’s generator which is more effective than a list comprehension but if it’s broken, we need a regression test to make sure the error doesn’t come back.

Out of curiosity, is numpy involved? Googling that error brings up a bunch of it.

@ricardopinto

This comment has been minimized.

Copy link
Contributor Author

ricardopinto commented Jul 7, 2018

I tried to pinpoint the code that triggers the issue, but to no avail. There's plenty of threads and processes in execution time, and it seemed to happen when we created a thread that enqueued a message on a periodic basis, it had nothing related to logging, numpy or anything complex. I even reduced all its code to a simple return statement and the issue still happened. If we don't start the thread, no error happens.

class PeriodicTrigger(Thread):
    def __init__(self, queue: Queue, interval: float):
        super(PeriodicTrigger, self).__init__()
        self.queue: Queue = queue
        self.interval: float = interval
        self.should_stop: bool = False

    def run(self):
        while not self.should_stop:
            now: float = time()
            next_run_at = now + self.interval
            sleep(next_run_at - now)
            self.queue.put('a_message')

As you can see, there's nothing particular about this thread.

Numpy is indeed a requirement, but I couldn't see any direct relationship in the traces or code.
I agree the generator should work just fine, but this was the only way to prevent the issue from happening.

@hynek

This comment has been minimized.

Copy link
Owner

hynek commented Jul 14, 2018

Looks like we’re not the only ones with this problem: kennethreitz/requests#4567

It seems like there’s something weird going on with frames, C extensions and threads. The unholy trifecta. :)

Would you mind changing [] to tuple() and see if it helps too? They’re a bit more efficient.

@hynek

This comment has been minimized.

Copy link
Owner

hynek commented Jul 28, 2018

Bump? 😇

@techdragon

This comment has been minimized.

Copy link

techdragon commented Dec 17, 2018

I'm getting quite a lot of log noise due to exceptions in this generator, so fixing this would be very welcome.

@hynek

This comment has been minimized.

Copy link
Owner

hynek commented Dec 17, 2018

@techdragon would you mind to test whether the proposed solution with tuple() instead of [] works then?

@ricardopinto

This comment has been minimized.

Copy link
Contributor Author

ricardopinto commented Dec 19, 2018

@hynek I tried tuple() instead of [] and it worked. Want me to update the code in the PR?

@hynek

This comment has been minimized.

Copy link
Owner

hynek commented Dec 22, 2018

That would be swell!

@ricardopinto

This comment has been minimized.

Copy link
Contributor Author

ricardopinto commented Jan 2, 2019

@hynek done. Happy new year!

@hynek hynek reopened this Jan 17, 2019

@hynek hynek merged commit 8933021 into hynek:master Jan 17, 2019

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to fd01af7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek

This comment has been minimized.

Copy link
Owner

hynek commented Jan 17, 2019

thanks and sorry for the delays!

@ricardopinto ricardopinto deleted the ricardopinto:18.1.0-fixed branch Jan 17, 2019

@ricardopinto

This comment has been minimized.

Copy link
Contributor Author

ricardopinto commented Jan 17, 2019

Excellent, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment