Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

When sharing IOLoop, Requests on port x are serviced in the wrong thread. #689

Closed
y-p opened this Issue Mar 4, 2013 · 10 comments

Comments

Projects
None yet
3 participants

y-p commented Mar 4, 2013

I ran into this issue while developing a library on top of tornado. The documentation clearly states that it's unsafe to call most methods from a different thread, but the handler doesn't call to any tornado methods. (and python's print not being thread-safe is irrelavent here).

Versions affected

2.4.1, git master dbff2b5, probably earlier version as well.

The scenario:

  • Multiple HTTPServer, each listening to a different port
  • Shared global IOLoop shared, via tornado.ioloop.IOLoop().instance().
  • All server have a handler with an identical URLSpec , "/a" in the example below
  • When invoked, the request handler merely prints the ident of the current thread
    to stdout.

Symptoms:

When issuing a request to a server at a specific port, the reported thread ident
is not constant, and cycles randomly through the all the threads of the other
servers who have a matching URLSpec.
I'm relying on per-server, thread-local data to be available to handlers and this
behaviour breaks that.

Expected behaviour

requsts made to a server on a given port would always be handled
from within the thread of that server, and none other.

Mitigation

Using multple IOLoops , rather then the global singleton loop, one per
server has so far eliminated these issues.

Steps to reproduce

The following script reproduces the problem, the number of servers
running only needs to be >=2, but more servers make the problem
more apparent.
After running the script, open up a browser to http://127.0.0.1:9000,
and repeatedly hit refresh. The console output will show the ident of
the thread serving the requiest, which is randomly cycling through threads.

#!/usr/bin/python
# -*- coding: utf-8 -*-
import threading
import errno
import time
from tornado.httpserver import HTTPServer
from tornado.web import  RequestHandler

class H(RequestHandler):
    def get(self):
        print( str(threading.current_thread().ident))

class SRV( threading.Thread):
    def __init__(self, port=None, address='127.0.0.1', **kwds):
        """
        If port== None, will pick one automatically
        """
        import tornado.web
        import tornado.ioloop

        super(SRV, self).__init__()

        self.name = "Server Thread"
        self.daemon = True

        # this is the important bit
        self.ioloop = tornado.ioloop.IOLoop().instance()
        self.application = tornado.web.Application(debug=False)
        self._server = HTTPServer(self.application)

        # vs. this, which behaves as I would accept
        # self.ioloop = tornado.ioloop.IOLoop()#.instance()
        # self.application = tornado.web.Application(debug=False)
        # self._server = HTTPServer(self.application,io_loop=self.ioloop)

        self._address_requested = address
        self._port_used = None
        self._address_used = None


    def run(self):
        import socket
        from tornado.web import URLSpec

        self.application.add_handlers("", [URLSpec('/a',H )]) # re-register everything

        for portnum in range(9000,10000):
            try:
                self._server.listen(portnum,'127.0.0.1')
                print('Server listening on port {0}'.format(portnum))
                self._port_used = portnum
                self._address_used = self._address_requested
                self.ioloop.start()

                return
            except socket.error as  e:
                if e.errno != errno.EADDRINUSE:
                    logger.error(str(e))
                    break

servers=[]
for i in range(10):
    servers.append(SRV())
    servers[-1].start()

while(True):
    time.sleep(1)  # block main thread til KBInterrupt
Contributor

ajdavis commented Mar 4, 2013

The behavior you see is certainly the behavior I'd expect. An IOLoop shared among N threads doesn't resume thread X when an I/O operation begun from thread X is ready—the IOLoop doesn't remember which thread began which operation.

The docs say, "Atypical applications may use more than one IOLoop, such as one IOLoop per thread, or per unittest case." Using one loop per thread is how I'd expect your application to work.

y-p commented Mar 4, 2013

The docs do say that, but do not explain when/why that's necessary, or what constitutes "ATypical".

If this behaviour is by design then I'm not sure what binding to different ports actually achieves.
Making a request on one port and ending up in the handler of a different application registered with
a different server running in another thread, that is listening on a different socket... Well that is exciting,
I admit, but a warning in the docs would have saved me some vigorous head-scratching.

Owner

bdarnell commented Mar 4, 2013

You're calling IOLoop.start() on the same instance from multiple threads, which is not allowed. You seem to have figured this out with the commented-out part that creates one IOLoop per thread; what do you hope to accomplish by sharing the IOLoop instead?

The requests are all going to the correct server, you just can't see the difference because the servers are all identical. You could see this by adding a keyword dict to the URLSpec to pass the server's identity into the handler.

The docs use the word "atypical" because most applications don't need to use threads at all. You can run multiple Applications and HTTPServers on the same IOLoop on a single thread. Because of the GIL, threads in python aren't as helpful as they are in other languages, so it's uncommon to use both an event loop and threads (except for specialized cases, like using a thread pool for DNS resolution or password hashing).

y-p commented Mar 4, 2013

I did the check you suggested with the keyword dict. yep. There's still the
thread context issue though.

Thanks for pointing out the misuse of start(). I changed things so that
the global ioloop is started once after launching all the threads, and now
I get the same thread ident reported no matter what port the request
comes in on. That's still not what I would expect.

I was sharing the loop because I didn't realize there's a reason not to, and I
still don't understand the behaviour I'm seeing. OTOH, I now realize I can avoid
threads all together assuming it's actually possible to add and remove servers
from a running IOLoop (presumably, the global one) dynamically in a safe way.
Is it?

Owner

bdarnell commented Mar 5, 2013

It's not just start(). server.listen() also calls in to the IOLoop (which was configured when the server was created), so it's not safe to call listen in other threads either. In general, everything that touches the network doesn't do anything directly, it asks the IOLoop to do something for it, so that's why everything gets moved back to the IOLoop's thread. Event-loop-based systems are usually used instead of threads, not with them, so it looks like you should just do everything in the main thread. (and yes, you can start and stop servers freely on the same IOLoop)

@bdarnell bdarnell closed this Mar 5, 2013

y-p commented Mar 5, 2013

I would have found it helpful if the docs stated something like:

If you choose to design your application using multiple threads,
you should use an IOLoop per thread rather then have them share 
the global one.

as an example of an "Atypical" application.

Owner

bdarnell commented Mar 6, 2013

But that gets the reasoning backwards - if you need multiple IOLoops you have to give each one its own thread, but if you need multiple threads you don't have to give each one an IOLoop. If what you need is threads you're probably better off with a concurrent.futures.ThreadPoolExecutor.

y-p commented Mar 6, 2013

ok, that's clear now.

I don't understand the ThreadPoolExecutor reference, surely there's no way to avoid IOLoops if
i'm using tornado, which is a requirement and i'd like to take advantage of t's web framework
features which a good fit.

Just to recap, If threads do all unsafe things via add_callback() as you explained,
request handlers always wakeup in their server's thread context?

Owner

bdarnell commented Mar 8, 2013

You need one IOLoop to use tornado; you usually don't need more than one. My recommendation is to use one IOLoop on the main thread, and then use ThreadPoolExecutors for any additional threads you need.

A server's "thread context" is its IOLoop; request handlers will always be run on the IOLoop their server is associated with. This normally means that all request handlers run on the main thread. If you're running ten servers and ten IOLoops on ten threads, then each server's requests will be run on that thread's IOLoop. But that's nearly alway a lot of added complexity for almost no benefit, so unless you have a specific reason for running IOLoops in multiple threads you should stick to one loop in the main thread.

y-p commented Mar 8, 2013

Thank you, all clear now.

@Rafiot Rafiot referenced this issue in kgaughan/uwhoisd Aug 23, 2016

Open

Fix major bug with tornado #30

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