9358 archer0001 sendbuffer #946
Conversation
Updated send and accept to no longer attach the buffer to the completion event, saving memory. Current tests pass.
|
I'm unsure why the AppVeyor build is failing. I installed all test deps and 64 bit python 3.6 in a test VM. Running trial twisted.web.tests yields a successful test. |
| @@ -225,7 +225,7 @@ cdef class CompletionPort: | |||
|
|
|||
| obj = None | |||
| if ov: | |||
| obj, ignored = <object>ov.attached | |||
| obj, _ = <object>ov.attached | |||
There was a problem hiding this comment.
Not really. I can remove that change if you feel it's unnecessary.
There was a problem hiding this comment.
That change is generating a big diff and the tests in this PR are failing :) ...so I don't know if this has any wild/crazy link with the test failures :)
As long as the tests are green I am ok with this cleanup :)
| @@ -0,0 +1 @@ | |||
| twisted.internet.iocpreactor.send no longer attaches send buffer to completion event, saving memory. | |||
There was a problem hiding this comment.
Is twisted.internet.iocpreactor.send a real thing :)
twisted.internet.iocpreactor is a package and I don't see any send there
https://github.com/twisted/twisted/blob/twisted-17.9.0/src/twisted/internet/iocpreactor/__init__.py
There was a problem hiding this comment.
I need to update the newsfragment. It should be twisted.internet.iocpreactor.iocpsupport.send.
Yes. I don't know. It seems that the error is constant on Appveyor Unfortunately, due to lack of manpower (to implement the required infrastructure) we don't yet run all Buildbot tests for branches which are outside of the main Twisted repo. I have triggered the tests on Builbot...to see what we get: https://buildbot.twistedmatrix.com/boxes-unsupported?branch=9358-archer0001-sendbuffer https://buildbot.twistedmatrix.com/builders/windows7-32-py3.6/builds/81/steps/iocp/logs/problems The test also fail on the Buildot. But this might be related to #948 so we should first fix http://twistedmatrix.com/trac/ticket/9353 and then come back to this cleanup? What do you think? Thanks! |
There was a problem hiding this comment.
Thanks for working on this.
Tested as a side effect of other reactor tests.
I think that the failure that we see here might be cause by https://twistedmatrix.com/trac/ticket/9353
Can we try to fix https://twistedmatrix.com/trac/ticket/9353 first and after that merge this?
I have no idea why the failure is triggered here.
Maybe include part of this code in https://twistedmatrix.com/trac/ticket/9353 so that we can see a test failing an Appveyour, and then after you apply your fix we can see that the test pass.
Does that make sense?
Thanks again!
|
Yup, I'm completely fine with resolving 9353 before hitting this. |
|
There are merge conflicts and I can't auto-resolve them (possibly because of the age of this PR) but this would certainly still be good to get fixed. 9353 (or its equivalent) has been resolved, and our current Windows build situation is much more reliable, so now might be a good time to try! |
https://twistedmatrix.com/trac/ticket/9358
Tested as a side effect of other reactor tests.
Additionally updated build script to work with both visual studio and mingw builds, maintaining default behaivor of using mingw.