only trampoline when we block #314

davidszotten · 2016-05-16T17:06:22Z

update

bugfix: only trampoline after catching a "blocking" ioerror. trampolining might otherwise fail, e.g. if using epoll and the fd happens to be a regular file (would never block)

test thanks to @Haypo from #257

original description, kept for posterity

wasn't able to figure out how to reproduce in a test, but can be trigged with a pytest test:

import eventlet
eventlet.monkey_patch()

pytest_plugins = "pytester"


def test_foo(testdir):

    testdir.makepyfile(
        """
        def test_foo():
            print('foo')
        """
    )
    result = testdir.runpytest()
    assert result.ret == 0

hangs on py3 without this patch

davidszotten · 2016-05-16T17:08:24Z

eventlet/greenio/py3.py

            except OSError as e:
                if get_errno(e) not in SOCKET_BLOCKING:
                    raise IOError(*e.args)
-            self._trampoline(self, read=True)


this works either way, but i think having it inside the except clause is more semantically correct (and copying this is what caused the error below)

temoto · 2016-05-16T20:38:15Z

Thanks for patch.
How to read this test? I fail to understand what exactly does it verify.

davidszotten · 2016-05-16T21:16:59Z

do you mean test_foo? That's distilled from a test testing a pytest plugin. Without the patch in this pr, running that test the runner just hangs. The patch stops the hanging.

Tried but failed to trigger the same issue from the eventlet test suite. Not suggesting the above should be added, just included it for reference if anyone else wants to try to to reproduce it.

davidszotten · 2016-05-20T09:24:38Z

do you need anything else here?

temoto · 2016-05-20T12:27:03Z

Not with code, but field testing would be nice.

davidszotten · 2016-05-20T16:17:41Z

is that something you are looking for from me?

jstasiak · 2016-05-20T18:00:28Z

I think we should have a reliable unit test included in the same patch as the issue seems nontrivial.

davidszotten · 2016-05-21T17:58:31Z

@jstasiak as i mentioned, i haven't been able to reproduce in a test scenario. i can trigger the bug using pytest and the minimal example in the description. any help writing a test would be appreciated

temoto · 2016-05-22T19:32:03Z

I take test.

Anybody willing to run this version on their systems - much appreciated.

davidszotten · 2016-05-23T13:21:02Z

To add another datapoint: I had been testing this on os x, which is where pytest hangs without this patch. On linux i instead get a Second simultaneous read exception.

temoto · 2016-05-23T17:37:05Z

No offense, I'm on OSX desktop too, but it's kind of irrelevant. So new symptom is second read. Which may be related to these issues: #94 #98 #273 so watch out, we already had regressions related to this code.

davidszotten · 2016-05-23T22:23:19Z

So if i understand correctly, this change just disturbs things enough (in my particular case) to avoid a bug elsewhere. In that case, this is a simple performance improvement (no need to trampoline until we block), and the existing test should be sufficient.

davidszotten · 2016-06-01T11:09:17Z

ok. some more digging: the simultaneous read might be a red herring (or an effect of something). hadn't noticed there was another exception being raised earlier:

>                   self.poll.register(fileno, mask)
E                   PermissionError: [Errno 1] Operation not permitted

looks like we are getting

EPERM The target file fd does not support epoll. This error occur if fd refers to, for example, a regular file or directory.

travis run:
https://travis-ci.org/mattbennett/eventlet-pytest-bug/jobs/133923901

do you know what causes this?

davidszotten · 2016-06-01T12:33:13Z

ok, now that i have something better to look for, i guess i'm just hitting #256, fixed by #257 (merge conflicts are just that multiple tests were added in tests/patcher_test.py). happy to rebase that branch and raise a new pr if you prefer?

could still (separately) take the slight perf improvement in this pr, though that's less important

temoto · 2016-06-02T11:53:27Z

Thanks for investigation. Let's put this trampoline optimisation on hold for a while and continue with #257.

davidszotten · 2016-06-02T13:22:12Z

so i'm back to thinking this is the proper fix for #256. apologies for the amount of back and forth, but i think my understanding of what's going on is slowly improving. i've incorporated the test from #257 which passes with this fix (and fails without it).

davidszotten · 2016-06-14T12:48:43Z

hi. do you need anything else from me to move this forward?

mattbennett · 2016-06-27T20:01:34Z

It would be great to see a fix for this, or #257

temoto · 2016-07-01T01:08:59Z

tests/isolated/regular_file_readall.py

@@ -0,0 +1,42 @@
+import eventlet
+eventlet.monkey_patch()


Please, follow this pattern for isolated tests:

__test__ = False if __name__ == '__main__': import eventlet # and everything else

monkey_patch() at module level is a hidden bomb in a file that is potentially imported by test runner collector.

thanks, hadn't noticed that the standards had moved since that test was written. updated now

temoto · 2016-07-01T09:31:47Z

Field testing is very welcome.

Thanks, @davidszotten

davidszotten · 2016-07-01T09:42:39Z

thanks. nameko is a decent sized open source library that makes heavy use of eventlet. there is already a travis job that runs against the latest released version of eventlet, but if it would help we could probably add one against eventlet master (with failure allowed)

temoto · 2016-07-01T10:31:30Z

Yes, please! CI of different code against master would be super awesome.

only trampoline when we block

ef8589d

davidszotten reviewed May 16, 2016
View reviewed changes

davidszotten mentioned this pull request Jun 2, 2016

remove "protected" managed threads nameko/nameko#324

Merged

davidszotten mentioned this pull request Jun 2, 2016

Fix GreenFileIO.readall() for regular file #257

Closed

davidszotten added 2 commits June 2, 2016 13:09

add test from haypo:readall_regular

e117fca

pep8

a953777

temoto reviewed Jul 1, 2016
View reviewed changes

temoto added the importance-enhancement label Jul 1, 2016

temoto added this to the v0.20 milestone Jul 1, 2016

update isolated test style

02ceec0

temoto merged commit 863a1b7 into eventlet:master Jul 1, 2016

mattbennett mentioned this pull request Jul 6, 2016

add master eventlet build nameko/nameko#340

Merged

mattbennett mentioned this pull request Jul 27, 2016

Pytest eventlet coverage mess mattbennett/nameko#1

Closed

only trampoline when we block #314

only trampoline when we block #314

Uh oh!

Conversation

davidszotten commented May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

update

original description, kept for posterity

Uh oh!

davidszotten May 16, 2016

Choose a reason for hiding this comment

Uh oh!

temoto commented May 16, 2016

Uh oh!

davidszotten commented May 16, 2016

Uh oh!

davidszotten commented May 20, 2016

Uh oh!

temoto commented May 20, 2016

Uh oh!

davidszotten commented May 20, 2016

Uh oh!

jstasiak commented May 20, 2016

Uh oh!

davidszotten commented May 21, 2016

Uh oh!

temoto commented May 22, 2016

Uh oh!

davidszotten commented May 23, 2016

Uh oh!

temoto commented May 23, 2016

Uh oh!

davidszotten commented May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidszotten commented Jun 1, 2016

Uh oh!

davidszotten commented Jun 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

temoto commented Jun 2, 2016

Uh oh!

davidszotten commented Jun 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidszotten commented Jun 14, 2016

Uh oh!

mattbennett commented Jun 27, 2016

Uh oh!

temoto Jul 1, 2016

Choose a reason for hiding this comment

Uh oh!

davidszotten Jul 1, 2016

Choose a reason for hiding this comment

Uh oh!

temoto commented Jul 1, 2016

Uh oh!

davidszotten commented Jul 1, 2016

Uh oh!

temoto commented Jul 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidszotten commented May 16, 2016 •

edited

Loading

davidszotten commented May 23, 2016 •

edited

Loading

davidszotten commented Jun 1, 2016 •

edited

Loading

davidszotten commented Jun 2, 2016 •

edited

Loading