python-hyper / hyper

HTTP/2 for Python.
http://hyper.rtfd.org/en/latest/
MIT License
1.05k stars 191 forks source link

Possible deadlock with HTTP20Connection #275

Closed plucury closed 7 years ago

plucury commented 8 years ago

HTTP20Connection.connect() acquires _lock when it was started, and try to acquire _write_lock in _send_preamble(). However HTTP20Connection.request() acquires _write_lock first, then it will try to acquire _lock in method _new_stream() and connect() . This should cause deadlock if there are two threads call them at the same time.

I'm not sure if it is possible to call connect() and _new_stream() before acquire _write_lock in connect() to avoid it.

Here is a simple script will stuck on deadlock: https://gist.github.com/plucury/f380d191ca7cc9f920e3f6d09dd5fefa

Lukasa commented 8 years ago

Hrm.

That definitely seems like a problem. Based on the rules documented in the comment, that means that request() needs to acquire _lock first, before it acquires _write_lock. I think the way we want to do that is to hoist connect() calls out if we can.

Probably the way we need to deal with this is that we need to stop using putrequest and endheaders directly in the code. Likely we need to turn those into wrapper methods that grab the correct locks, and then have internal methods that don't grab the locks at all so that we can manage them ourselves in request.

plucury commented 8 years ago

@Lukasa How about add a specific lock to synchronize stream creation/deletion instead of using _lock? If it is so, we can acquire it when we already held _write_lock or _read_lock without deadlock issue.

Lukasa commented 8 years ago

@plucury That's what _lock is: it's the specific lock that synchronizes stream creation/deletion. For a related reason it also synchronizes connection creation.

plucury commented 7 years ago

fixed