Skip to content
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

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

Closed
wants to merge 8 commits into from

Conversation

sponce
Copy link

@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 7 commits April 18, 2011 11:57
…eady while its internal state is not yet fine
… This server performs better than the ThreadedServer as it does not have to allocate a new thread for each request
Conflicts:
	rpyc/utils/server.py
…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
…ing 'NoneType has no attribute async_request' errors under load
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.
@tomerfiliba
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

@sponce, any news on that?

@tomerfiliba tomerfiliba closed this Jun 9, 2011
@tomerfiliba
Copy link
Collaborator

@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 pushed a commit to akruis/rpyc that referenced this pull request Mar 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants