Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update recvuntil #29

Merged
merged 2 commits into from
Mar 25, 2024
Merged

Update recvuntil #29

merged 2 commits into from
Mar 25, 2024

Conversation

rqdaA
Copy link
Contributor

@rqdaA rqdaA commented Mar 24, 2024

Update recvuntil

What's this Pull Request for?

Choose one or more of the following items.

  • Bug fix
  • Add/Remove feature
  • Cleaning code (including optimization/typo fix)
  • Others

Please explain the detail here:
Bug fix: Do not sleep when received the delim.
Feat: Added sleep_time argument.

What have you done so far?

Tell me what you've tested to assure your change is right.

  • All tests passed on your machine (python -m unittest)
  • Added test cases for new feature
  • Nothing / Test not required
    (It's not mandatory to check even one of them, but test cases make it easy for the author to review your PR.)

Comment

recvuntilが成功したかどうかに関わらずsleep(0.01)を挟むのはパフォーマンス上の問題があるので、成功した際にはsleepを行なわないようにしました。また、sleepの長さを引数で設定できる様にしました。

疑問点としてそもそもdelimが返ってくるまでrecv(size, timeout=-1)を回しているのでここでブロッキングが起こるはずで、追加でsleep
を入れる理由が分かりませんでした。

@ptr-yudai
Copy link
Owner

PRありがとうございます。
ブロッキングせずに空バイト列を返す_recvに対して、一定時間応答がない場合にCPU使用率を抑えるために昔sleepを挿入しました。
_recvが失敗した場合にのみsleepを入れる本修正は良いと思います 👍

疑問点としてそもそもdelimが返ってくるまでrecv(size, timeout=-1)を回しているのでここでブロッキングが起こるはずで、追加でsleepを入れる理由が分かりませんでした。

Tubeはいろんな通信に対する共通実装となっていて、プロセスやソケットはこれを継承して_recv_sendを実装する形になっています。ここで、_recvがブロックすることを仕様上保証していないので、先述のような問題が発生しました。

歴史上の経緯から他にもsleepを使ったままになっている箇所があるので、追って修正します。

@ptr-yudai ptr-yudai merged commit f27c17a into ptr-yudai:master Mar 25, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants