Allow tests requiring multiple servers at once #52
Conversation
Based on the need we had we used pytest-httpserver to test multiple server instances running at once. After checking documentation and background we found that on the purpose of the single was to override start-up time on werzeuk server library. We confirmed that that is not the case anymore at least at Werkzeug==1.0.1.
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
==========================================
- Coverage 95.58% 92.99% -2.59%
==========================================
Files 3 3
Lines 430 414 -16
==========================================
- Hits 411 385 -26
- Misses 19 29 +10
Continue to review full report at Codecov.
|
|
Thanks for the PR. Regarding
The background documentations (https://pytest-httpserver.readthedocs.io/en/latest/background.html#server-starting-and-stopping) says that the stopping is taking much time.
According to my experience it it still an issue with werkzeug 1.0.1. Running make quick-test takes significantly more time when run with your commits compared to origin/master. On my machine it is 10.59s vs 2.92s and the time difference increases with the amount of server restarts (which is proportional to the number of test cases). I completely understand that you want multiple servers, but I think this is supported the released versions of pytest-httpserver. If you want to run a new server instance, you can create your own fixture which creates a new httpserver instance, and thereby you can avoid using the provided fixture. According to readme: with HTTPServer() as httpserver:
# set up the server to serve /foobar with the json
httpserver.expect_request("/foobar").respond_with_json({"foo": "bar"})
# check that the request is served
print(requests.get(httpserver.url_for("/foobar")).json())This is very similar to the code you have done in test_multi.py: it does not use the httpserver fixture provided by the library but it uses its own server. According to my best knowledge, this should work with the current code on origin/master and not requires changing the plugin's code - so the users which needs only one server will be still happy as the performance remains the same and on the other hand you can run as many instances of the server as you want. |
I wouldn't had suggested it if I would know the problem persists, I'm sorry, according to my tests, the code with and without the patch took the same time. I known it doesn't prove anything, but check ticket #50. Is it possible that on windows there is no such problem?
[..]
Sure, I have already made something like that, thanks anyway! (Only thing was that I didn't found it in the readme or I missed it there). Thanks for your time! |
|
Aww, first: I'm terribly sorry, I completely missed the issue you have created.. My mistake. I haven't yet realized that there's anyone using it on windows, so it was trivial for me that if this occurs on Linux it will occure for everybody, but this is not true. I think I should add tests for windows in github actions from now to not break the library on your system. I've did a little bit of research of what is happening and this is the code which triggers the issue: import time
from threading import Thread
from werkzeug.serving import make_server
# this never gets called
def application():
pass
def main():
server = make_server("127.0.0.1", 9999, application)
thread = Thread(target=lambda: server.serve_forever())
thread.start()
t1 = time.time()
server.shutdown()
print(time.time()-t1)
thread.join()
if __name__ == "__main__":
main()For me this prints out a value near to 0.5 seconds. Here it creates a loop, which waits for some activity on a file descriptor for maximum poll_interval time, which is 0.5 seconds. When it times our or there's some activity, it checks the shutdown flag and if it is True it breaks out from the loop. Probably the selector works differently on windows as it a completely different flavour (I don't know how windows works with this, so this is just a guess). Anyway, I think I'll update the background.rst with this finding. |
|
Hello! Don't worry about the issue, and thanks again for your time. Two different things: First, I think the problem with/in my windows isn't actually at close but at open/connection. Changing this will fix it however I'm not sure this is something someone would like to do (I would probably be a misconfiguration in my box): - DEFAULT_LISTEN_HOST = "localhost"
+ DEFAULT_LISTEN_HOST = "127.0.0.1"The close/shutdown delay issue: it is bad to find out it is base system (python) problem :-( . One possible, perhaps not too happy workaround is to close the socket on before hand: def stop(self):
"""
Stop the running server.
Notifies the server thread about the intention of the stopping, and the thread will
terminate itself. This needs about 0.5 seconds in worst case.
Only a running server can be stopped. If the sever is not running, :py:class`HTTPServerError`
will be raised.
"""
if not self.is_running():
raise HTTPServerError("Server is not running")
+ self.server.server_close()
self.server.shutdown()
self.server_thread.join()
self.server = None
self.server_thread = NoneOn the not expected side effect side, it would happen that about half of the time, the thread which is at .. Anyway.. with those two changes, the tests in windows will run in 3seconds : Oh.. the other broken thing is the ssl test.. because the default host in this case was ìp`-address based instead of localhost.. I think it would be acceptable to have that one in particular back to localhost because delay in only one tests is acceptable and better than losing one test. But, again, I'm not sure about the change from name to ip.. |
|
Regarding the hostname issue: to by best knowledge the host and port are passed to the bind() call of the socket API, which takes the IP address, and I guess python is very kind and does the name resolution behind the scenes before making the call to Maybe it takes long on windows, but it sounds very strange. I need to make attempts to re-produce this on my windows, but again I'm far from a windows expert so don't expect me to dig into the source code there. Regarding server_close, I find it a bit risky thing to do so, the documentation explicitly specifies that shutdown() needs to be called:
While server_close is something you want to override, I think:
What I was experimenting is that running a different wsgi server behind werkzeug, to be honest socketserver is not the cutting edge technology, this is specified in the documentation as well, but pytest is not production and it is good to reduce the dependencies (and it looked ok at the beginning to not create anything complicated). But again, running multiple servers works can be done, you are not obliged to use the provided |
I've found where the "problem"(?) is on windows.. or at least, what is happening. :-) This line is where client iterates over all interfaces trying to connect Actually I have more interfaces in linux than in windows:
The problem is the order.. or the timeout as it takes 5seconds to discard ipv6 and try to connect to '127.0.0.1' in ipv4.
I think I wasn't clear. I didn't mean to replace server.shutdown() !! My suggestion was to call server.socket_close() just before the server.shutdown(). By doing this, you make it exit select call and and then prevent new loop by calling shutdown().. There is a little race condition where the shutdown isn't call fast enough. In that case, select() is called again and the socket is invalid which will produce an exception which will raise up to serve_forever.. it could be ignored here: def thread_target(self):
"""
This method serves as a thread target when the server is started.
This should not be called directly, but can be overridden to tailor it to your needs.
"""
try:
self.server.serve_forever()
except (check-which-exception):
passI think that the race-condition could be prevented if "closing/shutdown" signaling state variables in SocketServer were not private variables (or by work-around the name mangling).. in order to mark the shutdown state before the socket close and the shutdown. That would prevent the race condition.
I think it makes sense!
Yes yes, we have that already addressed, thank you!! I just would like that it be easier to get different servers .. and if in retribution for your lib I can help you find out something interesting, it would be win-win! Thanks! |
|
That's weird, it seems like a client error for first first look. If it tries to connect to ipv6 address, why the fall-back to ipv4 takes too long? And how it relates to the address you specify for bind? Oh, maybe "localhost" is resolved to both ipv6 and ipv4, and in such case werkzeug listens on both address family? On my Linux, if I specify "localhost" it only listens on ipv4. I'm almost certain that if you specify 127.0.0.1 then it will pick up ipv4-only. I understood that you want to call both server_close and shutdown, but from the documentation it seemed for me that server_close should not be called from outside. But looking at the code in socketserver.py, this may do the trick, as it closes the server socket, which terminates the selector in the thread so that 0.5 second waiting time is eliminated. |
Remove Plugin.SERVER singleton in order to make http-server reusable
Based on the need we had we used pytest-httpserver to test multiple server instances running at once. After checking documentation and background we found that on the purpose of the single was to override start-up time on werzeuk server library. We confirmed that that is not the case anymore at least at Werkzeug==1.0.1 (so we need to update documentation too).