public inbox for [email protected]
 help / color / mirror / Atom feed
* [GIT PULL] ncns update (chunked request body support)
@ 2022-08-25  9:40 Ammar Faizi
  2022-08-25  9:57 ` Alviro Iskandar Setiawan
  0 siblings, 1 reply; 10+ messages in thread
From: Ammar Faizi @ 2022-08-25  9:40 UTC (permalink / raw)
  To: Alviro Iskandar Setiawan
  Cc: Kanna Scarlet, Muhammad Rizki, GNU/Weeb Mailing List

Hi Alviro,

Sorry for sending you a pull request directly this time. I am in a rush
and don't have much time to get this sorted. I spent my day debugging
this. So in a non-chunked request, the body is fully written before the
request even goes in a connected state. So the calling sequence for a
non-chunked request is:

-> CreateNet()
  -> SetURL()
   -> SetRequestHeader()
    -> SetPayload() # request body goes here
     -> Start() in SQE
     --- cr on connected
     --- cr on response started
      -> Read() in SQE
     --- cr on read completed

But in a chunked request, the body should be fully written before we
wait for "on response started" from the Chromium net event. This is
because HTTP works that way. The request should go first before the
client waits for the response. The calling sequence that made the SQE
stuck was like this:

-> CreateNet()
  -> SetURL()
   -> SetRequestHeader()
    -> StartChunkedBody()
     -> Start() in SQE
     --- cr on connected
     --- the ring waits for "on response started"
       -> Write()
       -> Write()
       -> Write()
       -> Write()
       -> Write()
       -> Read() in SQE
       -> Write()
       -> Write()
     --- wait for "on read completed"
     ^^^ stuck here

That calling sequence is wrong, because the read will never complete
before the data stream class in the Chromium reaches its EOF (IOW, the
SetIsFinalChunk() [1] is called). That being said, implies "wait for 'on
read completed'" must only be done after the write operation has finished.

I guess between Friday and Saturday I can ship this feature properly.

Please pull!

[1]: https://source.chromium.org/chromium/chromium/src/+/109fd67d081a16dc8564bd15c146a78f4e068559:net/base/upload_data_stream.h;l=119-121

The following changes since commit 7914975d89b6cb947de27b17df13b2f4830197a1:

   Merge branch 'dev.ring_cleanup' (2022-08-18 05:48:50 +0700)

are available in the Git repository at:

   https://gitlab.torproject.org/ammarfaizi2/ncns.git tags/chnet_body_chunked.2022-08-25

for you to fetch changes up to 77bbcc903899016f3e99ce499ded2056cea77a03:

   chnet: Completely refactor again (2022-08-25 15:51:30 +0700)

----------------------------------------------------------------
chnet_body_chunked.2022-08-25

----------------------------------------------------------------
Ammar Faizi (23):
     chnet: Add initial request body support
     chnet: node: Add set_user_data support on SQE
     tests/js/ring: Update the unit test to utilize set_user_data
     binding.gyp: Add `-ggdb3` flag for better debugging experience
     binding.gyp: Add `-Wno-enum-constexpr-conversion` flag
     chnet: node: Add set_method function to set HTTP method
     chnet: node: Add get_error function to return the error string
     chnet: node: Add set_payload function to set HTTP req body
     tests/js/ring: Add simple HTTP POST request example in NodeJS
     chnet: Split construct URL req creation into a new function
     chnet: Add set request header support
     chnet: node: Fix unused variable warning
     chnet: node: Add set request header function in NodeJS
     tests/js/ring: Add more set header function test
     chnet: node: Don't use static counter for data ID
     tests/js/ring: Add JavaScript class wrapper example
     chnet: Initial chunked request body support
     chnet: Rework the chunked request body interface
     chnet: ring: Refactor the ring completely
     chnet: Use busy-waiting for signal waiter
     chnet: ring: Bump max_entry to 2G
     tests/cpp: Delete basic.cpp as it's no longer relevant
     chnet: Completely refactor again

  binding.gyp         |   4 +-
  chnet/WorkQueue.cc  |  17 +-
  chnet/WorkQueue.h   |   5 +
  chnet/chnet.cc      | 239 +++++++++++++----
  chnet/chnet.h       | 215 +++++++++++++--
  chnet/chnet.old.cc  | 582 ++++++++++++++++++++++++++++++++++++++++
  chnet/chnet.old.h   | 236 ++++++++++++++++
  chnet/chnet_node.cc | 564 +++++++++++++++++++++++++++++---------
  chnet/chnet_node.h  |   6 -
  chnet/chnet_ring.cc | 493 ++++++++++++++++------------------
  chnet/chnet_ring.h  | 200 ++++++++------
  chnet/common.h      |  20 +-
  tests/cpp/basic.cc  |  97 -------
  tests/cpp/ring.cc   | 536 +++++++++++++++++++++++++++++-------
  tests/index.php     |  14 +
  tests/js/ring.js    | 405 ++++++++++++++++++++++++----
  16 files changed, 2797 insertions(+), 836 deletions(-)
  create mode 100644 chnet/chnet.old.cc
  create mode 100644 chnet/chnet.old.h
  delete mode 100644 tests/cpp/basic.cc

-- 
Ammar Faizi

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-08-26 10:37 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-25  9:40 [GIT PULL] ncns update (chunked request body support) Ammar Faizi
2022-08-25  9:57 ` Alviro Iskandar Setiawan
2022-08-25 18:36   ` Ammar Faizi
2022-08-26  1:34     ` Alviro Iskandar Setiawan
2022-08-26  1:39       ` Alviro Iskandar Setiawan
2022-08-26  1:53         ` Ammar Faizi
2022-08-26  6:24           ` Ammar Faizi
2022-08-26  8:03             ` Ammar Faizi
2022-08-26  8:07               ` Ammar Faizi
2022-08-26 10:37                 ` Alviro Iskandar Setiawan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox