Fixed and added regression test for an IOStream bug that was introduced in 2.3 #561

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@bergundy

where the IOStream._close_callback would never be
called if there were pending reads.

@bergundy bergundy Fixed and added regression test for a bug that was introduced in 2.3
where the IOStream._close_callback would never be
called if there were pending reads.
62d5ee2
@bdarnell bdarnell added a commit that closed this pull request Jul 14, 2012
@bergundy bergundy Fixed and added regression test for a bug that was introduced in 2.3
where the IOStream._close_callback would never be
called if there were pending reads.

(Cherry-picked from #561 with
amendments for compatibility with python 2.5-2.6 -bdarnell)

Closes #561.
5edd8da
@bdarnell bdarnell closed this in 5edd8da Jul 14, 2012
@bdarnell
Member

It looks like this test is timing-sensitive and has been failing intermittently. :( (observed on OSX Lion, appears to happen ~10% of the time, on all python versions). Could you take another look at this test and see if you can make it deterministic? If you can't reproduce the problem I'll look at it later.

@bergundy

IIUC, The test fails on Lion because the last wait() times out.

Seems strange that it takes the OS more than 5 seconds to notify the IOLoop that the server connection is closed.
Does this make sense to you?
Is this what you see using strace?

I based the test on a timer because I didn't find a better way to do it w/o getting into iostream/ioloop internals or mocking.
If you have a better idea for the test, I'd be glad to hear it.

@bdarnell
Member

No, I'm not seeing the last wait() time out. I'm seeing the second read_until fail with a "connection already closed" exception.

I haven't tried anything yet, but I'm thinking maybe move the server.close from the write's callback to after the first wait, to make sure that the close isn't processed too soon. Would that work (and still cover the relevant bug)?

I think it's fine that the failure case for this test is just a timeout (plenty of tornado's tests are already like that).

@bergundy

I see, will try to get around to it in the next few days.

@bergundy

I moved the server.close() call from write the after write callback to just before the second read_until().
I don't have OSX so I can't really see if the test is accurate now.
On linux, with python2.7 the test fails on 2.3 and passes on latest git
bergundy@6fb0418

Do you have a way to test this?

@bdarnell
Member

Yeah, I just tested it on OSX and it looks like it fixes the problem. Thanks!

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