-
Notifications
You must be signed in to change notification settings - Fork 705
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
fix ResponseStream when used from other threads #1280
base: master
Are you sure you want to change the base?
Conversation
- if called from the worker thread (doing the epoll) this has no effect as it's cleared before going back to epoll - if called from another thread, it causes an unnecessary wakeup of the worker thread. The worker thread then throws an exception ("Assertion Error: could not find write data"), as the toWrite data is already removed. In case the flush call (asyncWriteImpl) cannot finish completely due to EWOULDBLOCK, it still sets NotifyOn::Write.
…sertion Even with the previous commit there is still a case where this can happen: - the worker thread returns from epoll with a write notification (e.g. due to a previous EWOULDBLOCK) - if at the same time another thread calls flush, which writes all data - then the worker thread finds an empty toWrite
…Impl By making sure the loop is exited. As NotifyOn::Write is set, the worker thread will then be notified when data can be written. Tested with code instrumentation by manually setting errno = EAGAIN, and checking that the worker thread then gets woken up and completes the write.
Thanks for the patch! Neatly separated in small, useful commits. Looks good to me :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bkueng. Please remember to bump the patch version, per our documentation. Otherwise it looks fine.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1280 +/- ##
==========================================
- Coverage 77.90% 76.28% -1.62%
==========================================
Files 57 60 +3
Lines 7912 10021 +2109
==========================================
+ Hits 6164 7645 +1481
- Misses 1748 2376 +628 ☔ View full report in Codecov by Sentry. |
I'm running into the same issue as #1006: I'm trying to flush a stream from another thread.
This PR adds 3 changes:
do not add NotifyOn::Write when flushing buffers
as it's cleared before going back to epoll
worker thread. The worker thread then throws an exception
(
"Assertion Error: could not find write data"
), as thetoWrite
data isalready removed.
In case the flush call (
asyncWriteImpl
) cannot finish completely due toEWOULDBLOCK
, it still setsNotifyOn::Write
.remove "Assertion Error: could not find write data" assertion
Even with the previous change there is still a case where this can happen:
(e.g. due to a previous
EWOULDBLOCK
)toWrite
prevent busy-loop in case of EWOULDBLOCK in asyncWriteImpl
By making sure the loop is exited. As
NotifyOn::Write
is set, the workerthread will then be notified when data can be written.
Tested with code instrumentation by manually setting errno = EAGAIN,
and checking that the worker thread then gets woken up and completes the
write.
Fixes: #1006