Some fix of netref + a better implementation of the ThreadPoolServer #42

Closed
wants to merge 8 commits into
from

Projects

None yet

2 participants

@sponce
sponce commented May 24, 2011

Note that only 2 commits are to be pulled out of the 7, I do not quite get why github insists in listing the 7, and in particular the merge. Anyway :

  • one fixes a subtle issue in netref, where a weak reference has become invalid when you try to use it. This leads to "NoneType has no attribute..." errors when using async requests under load
  • the other is a (much) better implementation of the ThreadPoolServer (see log message for details).
sponcec3 added some commits Apr 18, 2011
sponcec3 Fixed race condition by which a client may think the AsuncResult is r…
…eady while its internal state is not yet fine
00aea82
sponcec3 Implemented the ThreadPoolServer with a preallocated pool of threads.…
… This server performs better than the ThreadedServer as it does not have to allocate a new thread for each request
8901ae1
@tomerfiliba

could you elaborate how this change matters? is it because threads busy-wait on result.ready?

@tomerfiliba

nice work. for clarity reasons, i'd raise a ThreadPoolFull exception (or something like that) instead of AsyncResultTimeout.

sponcec3 added some commits May 4, 2011
sponcec3 Merge remote-tracking branch 'upstream/master'
Conflicts:
	rpyc/utils/server.py
80599ab
sponcec3 Better handling of full thread pool cases in the ThreadPoolServer cla…
…ss. Do not throw an exception, as this would trigger an exit from the server, but close the socket so that the client gets a connection refused error and log the situation
57fe482
sponcec3 Fixed missing check for existence of a weak reference. This was creat…
…ing 'NoneType has no attribute async_request' errors under load
bb0948b
sponcec3 More efficient implementation if the ThreadPoolServer.
The first implementation was dispatching connections relying on the fact that they were disappearing. In case there were persistent and more numerous than the number of threads, some connections were never served. On top, the server was exiting when overloaded.
The new version dispatches requests (in bulk for efficiency) and allows thus any type and any number of connections.
d4b26b9
sponcec3 Merge remote-tracking branch 'upstream/master' eec5085
@tomerfiliba
Owner

bb0948b: merged

d4b26b9: i see you're using select.poll, so this code won't run on windows. could you change it to select.select instead? that way it would be cross-platform.

57fe482: i'd guess d4b26b9 overrides this commit?

@sponce
sponce commented May 24, 2011

d4b26b9: i see you're using select.poll, so this code won't run on windows.
Well, indeed (just read the doc...). Not that I would try to run a high performance server on windows :-)
I'll see what I can do. The problem is I cannot test under windows anyway and the tests under linux were all done with poll. And these are not at all easy to do (they especially involved quite some hardware) so I will have a hard time redoing them....

57fe482: i'd guess d4b26b9 overrides this commit?
Yes it does, I had forgotten that one. You forget it too :-)

@tomerfiliba
Owner

don't worry about testing on windows too much -- testing it on linux is enough for me. i just want the code to be able to run on all platforms.

by the way -- you could write a "mock poll" object that relies on select when poll is not supported. e.g.

class MockupPoll(object):
    def register(self, fd, flags):
        if flags & select.POLLIN:
            self.rfds.append(fd)
        if flags & select.POLLOUT:
            self.wfds.append(fd)
    def poll(self, timeout):
        rfds, wfds = select.select(self.rfds, self.wfds, [], timeout)
        return unified list of fds and event mask

that way you can keep your code in tact and still be cross-platform.

@tomerfiliba
Owner

@sponce, any news on that?

@tomerfiliba
Owner

@sponce: i integrated your ThreadPoolServer. please review the commit (c1c3b5d) and rpyc/lib/compat.py. i replaced your wrappedPoll with a more functionally-complete mock.

could you write an automated test for it?

thanks for your help,
-tomer

@akruis akruis pushed a commit to akruis/rpyc that referenced this pull request Mar 22, 2012
@tomerfiliba cosmetic fixes + integrate patch by @sponce. closes #42 f1a6729
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment