I wonder if we should just bite the bullet and make the entire class threadsafe?

That is an end goal, but also looks like a big project. Just putting Rlocks everywhere is probably not going to cut it 😆.
In particular, there's quite a lot of future resolving. I'd say those callbacks would need to be put outside locked sections.

I missed one more race condition around IFR which just popped up in logs, easy addition.

I believe there is still a race here that could lead to protocol out of sync:

Thread A (with conn._lock) -> _protocol.send_request(request)
Thread B (with client._lock) -> conn.send_pending_requests(), including network I/O write
Broker -> processes request, sends response
Thread B (with client._lock) -> receives selector.EVENT_READ
Thread B (with conn._ifr_lock) -> checks conn.has_in_flight_requests()

Notice that Thread A has not yet placed an entry on its in_flight_requests dict, so Thread B will think this is a protocol out-of-sync error and close the socket.

Granted, this timing is extreme and seems highly unlikely. And, even if this did happen, your changes should prevent the "hanging" problem caused by the race between disconnect + ifr queue.

Nonetheless, I wonder if we should be reusing the existing conn._lock here? I think that would synchronize the protocol buffer, network I/O, and ifr tracking.

@dpkp
Good catch, I am actually reproducing this one too.
Yes, the solution is to extend self._lock until in_flight_requests is processed.

I don't think it would work to use self._lock everywhere, particularly in close, since it looks like it can get called from inside sections which are already locked (and I'd like to avoid a more expensive reentrant lock).

I hear you, but I am worried about performance if we use RLocks.

In older Pythons it would be significantly slower, and we have components still stuck to 2.7
https://stackoverflow.com/a/1977542
https://stackoverflow.com/a/5441992

I'll try from scratch and see if I manage to rewrite this using just a non-reentrant _lock, but it will have to include some more substantial changes.

This was great work! But I'm going to close in favor of the other PRs because I think we have a good path forward there.

I am worried about performance if we use RLocks. In older Pythons it would be significantly slower, and we have components still stuck to 2.7

Given how late we are in the python 2.7 lifecycle, I personally think we should be more concerned about getting the semantics correct and not worry too much about the python 2.7 performance.

The momentuum to EOL python 2.7 seems to have really picked up over the last 18 months... We've been experiencing this at my dayjob, seems like every other week a new 3p open source library announces they will quit supporting python 2.7 in 2020.

So I suspect what will happen is that companies/application owners will be faced with a choice to either migrate to python 3 or simply stop upgrading all their external libraries. And if they choose to stop upgrading, then whether kafka-python are using normal locks or RLocks won't matter because they'll still be on an old version.

Again, this isn't quite where we are at today, but it's coming very very quickly, so if we go for what's semantically correct, it will make long-term maintenance much easier.