select should not be used in WSClient because python-websockets does its own buffering

[PaulFurtado]

I'm hitting an issue where some exec calls made via the python client hang.

For background, I'm in the process of migrating my cluster from docker to cri-o and I have a suite of acceptance tests written in python that I run against the cluster to verify functionality. A good deal of of the tests exec commands inside pods and upon switching to cri-o, a certain command in the tests hangs/times out every time. Initially, I thought there was a bug in cri-o, but If the same command is executed via kubectl, it does not hang, so I started diving into the python client.

I believe I've tracked the problem down to the select call that is made in WSClient:

python-base/stream/ws_client.py

Lines 169 to 170 in 67a69d4

	r, _, _ = select.select(
	(self.sock.sock, ), (), (), timeout)

If I remove that select call, commands never hang.

I think that it's invalid to call select there because select is a system call that checks if a socket ready to be read/written, however, with websockets, we're several layers of abstraction away from the underlying system socket that select is checking and buffering is occurring at each of the layers. I suspect what is happening is:

1. recv_data_frame is called. The underlying recv call on the socket uses a fixed buffer size and receives multiple frames and recv_data_frame returns only one of them.
2. on the next iteration, we check if the underlying socket has any bytes available using select. It does not, but there is a frame waiting in the buffer in websocket-client that recv_data_frame would instantly return.

So I think the solution is to simply remove that select call, but I'm curious why it was added in the first place and whether removing it breaks some expectation. The only thing that I can think of is that it's being used to make read timeouts possible, however, if so, that is still buggy because the select could return if 1 byte is available to read, but then the recv calls in websocket-client would still block waiting on a complete frame.

@mbohlool what do you think?

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[bboe]

To update on this issue, I ran into an issue with select as documented here: #53 (comment)

My thought was to convert to poll (has potential problems as not all operating systems support poll (but maybe all that support this package do?). However, if this select isn't needed, then that simplifies things even more.

[PaulFurtado]

@bboe are you talking about epoll? If so, epoll has all the same problems as select here: the underlying socket's readiness is not equivalent to the websocket's readiness as the websocket is a logical construct with its own framing which the operating system doesn't know about

[bboe]

@PaulFurtado no, I was just going to convert to regular poll to remove the max file descriptor number of 1023 limit of select. However, as you've indicated here, it seems like simply removing the select is better, unless there is a need to have a timeout when no data is available.

Do you know who would need to sign off on such a change? I'm likely going to vendorize a version of this with select removed, but it'd be great to eventually be able to clean that up.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[zmc]

@bboe @PaulFurtado Curious, does one of you have a fix for this? I'd be happy to test it.

[PaulFurtado]

/remove-lifecycle rotten

@zmc you should be able to just remove the line linked above:

python-base/stream/ws_client.py

Lines 169 to 170 in 67a69d4

	r, _, _ = select.select(
	(self.sock.sock, ), (), (), timeout)

I've done this internally and AFAIK there haven't been negative side effects.

---

@bboe I confirmed you're definitely correct, after all these years, I didn't realize that about select and file descriptor numbers!

Do you know who would need to sign off on such a change? I'm likely going to vendorize a version of this with select removed, but it'd be great to eventually be able to clean that up.

Unfortunately, I'm also just a kubernetes user. It seems like issues on this repo aren't really answered, but maybe if we file an issue on the main kubernetes repo someone will take a look?

[bboe]

I've done this internally and AFAIK there haven't been negative side effects.

Removing the socket select is also what we have done without any ill effects. I am moving away from invoking K8 directly in favor of Google Cloud Run, so my desire to have this fixed is lessened.

I confirmed you're definitely correct, after all these years, I didn't realize that about select and file descriptor numbers!

That discovery absolutely blew my mind.

[TomasTomecek]

Having the same issue with same symptoms. Tracking issue in our project: packit/sandcastle#23