Re: [PATCH v1 17/22] chnet: Initial chunked request body support

[Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>]

On Sun, Aug 21, 2022 at 6:24 PM Ammar Faizi wrote:
> +int CHNetDataStreamChunked::ReadInternal(net::IOBuffer *buf, int read_size_arg)
> +{
> +       size_t read_size;
> +       char *ptr;
> +       int ret;
> +
> +       if (unlikely(read_size_arg < 0))
> +               return net::ERR_IO_PENDING;
> +
> +       if (unlikely(is_eof_ || !read_size_arg))
> +               return 0;
> +
> +       ret       = 0;
> +       ptr       = buf->data();
> +       read_size = (size_t)read_size_arg;
> +
> +       std::unique_lock<std::mutex> lk(buf_lock_);
> +       while (read_size) {
> +               struct net::CHNetDataStreamChunked::buf *b;
> +               size_t copy_size;
> +
> +               if (unlikely(!buf_queue_.size())) {
> +                       /*
> +                        * At this point, the buffer is empty.
> +                        *
> +                        * Wait for the buffer queue to be
> +                        * filled in.
> +                        */
> +                       being_waited_.store(true, std::memory_order_release);
> +                       buf_cond_.wait(lk, [this]{ return buf_queue_.size(); });
> +                       being_waited_.store(false, std::memory_order_release);
> +               }
> +
> +               b = buf_queue_.front();
> +               if (b->len_ == -(size_t)1UL) {
> +                       is_eof_ = true;
> +                       break;
> +               }
> +
> +               copy_size = b->len_ - cur_pos_;
> +               if (copy_size > read_size)
> +                       copy_size = read_size;
> +
> +               memcpy(ptr, &b->buf_[cur_pos_], copy_size);
> +               ptr += copy_size;
> +               ret += copy_size;
> +               read_size -= copy_size;
> +               buf_queue_.head_++;
> +       }
> +
> +       if (unlikely(is_eof_))
> +               SetIsFinalChunk();
> +
> +       if (unlikely(!ret && !is_eof_))
> +               return net::ERR_IO_PENDING;
> +
> +       return ret;
> +}

I think you shouldn't return net::ERR_IO_PENDING, just sleep on the
condition variable if the buffer is not ready yet, it is legal to
sleep on ReadInternal(), why spend time spinning on -EAGAIN?

You can end in OnReadCompleted() if you sleep on the condition
variable and wait for it to get signaled:
https://source.chromium.org/chromium/chromium/src/+/main:net/base/upload_data_stream.cc;l=82-87;drc=5eda14f193ef3e0131aad2f31ae172c0001b3f6d?q=upload_data_stream.cc&ss=chromium%2Fchromium%2Fsrc

tq

-- Viro