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

src,stream: improve DoWrite() and Write() #44434

Merged
merged 2 commits into from Oct 10, 2022

Conversation

ywave620
Copy link
Contributor

StreamBase::Write() calls StreamResource::DoTryWrite() and StreamResource::DoWrite() to do the job.
However the interface definition of StreamResource::DoWrite() is error-prone or at least confusing, which I guess leads to an wrong(but no effect fortunately) override, Http2Stream::DoWrite(). This PR takes care of these two issues and make the StreamBase::WriteString() more efficent in the case where the underlying stream is too busy to receive more data.

@nodejs-github-bot
Copy link
Contributor

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 29, 2022
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you include a test?

src/stream_base-inl.h Show resolved Hide resolved
@@ -2372,8 +2372,7 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap,
CHECK_NULL(send_handle);
Http2Scope h2scope(this);
if (!is_writable() || is_destroyed()) {
req_wrap->Done(UV_EOF);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RafaelGSS After investigation, I find out that this if statement should be turned to an assertion. Because the JS code make sure that whenever we do a write on a stream, the stream is writable and not destroyed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it to an assertion (CHECK) will crash the application whenever someone tries to write in a destroyed stream, is that expected?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no.

Copy link
Contributor Author

@ywave620 ywave620 Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS code in /lib will not let this happens. Without misusing the internals, the case that captured by this if can not happen. If you insist keeping this if, then apparently, this function should return an error code immediately and never invoke the cb, because this is what every asynchronous API should do in case an immediate failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that happens when misusing the internals, adding the CHECK make sense to me.

src/stream_base.cc Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2022
@nodejs-github-bot

This comment was marked as outdated.

Qard
Qard approved these changes Aug 31, 2022
@nodejs-github-bot
Copy link
Contributor

@ywave620
Copy link
Contributor Author

ywave620 commented Sep 5, 2022

https://ci.nodejs.org/job/node-test-binary-windows-js-suites/16604/
Two tests failed.

  • parallel/test-vm-timeout-escape-promise-2
  • parallel/test-worker-heap-snapshot

The first one failed on many other CI, either, as you can see in nodejs/reliability#368. The second one fails rarely, maybe triggered by some concurrency issue, more details in #44515

@ywave620
Copy link
Contributor Author

Hello, how should I move this PR forward? @mcollina

Clarify StreamResource::DoWrite() interface definition.
Fix error-prone Http2Stream::DoWrite().
Change StreamBase::Write() to allow caller to avoid
inefficient write behavior.
@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2022
@nodejs-github-bot
Copy link
Contributor

@RafaelGSS
Copy link
Member

Hi @ywave620, looks like those tests were fixed. Could you please rebase?

@mcollina mcollina added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 7, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 7, 2022
@nodejs-github-bot
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/44434
✔  Done loading data for nodejs/node/pull/44434
----------------------------------- PR info ------------------------------------
Title      src,stream: improve DoWrite() and Write() (#44434)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     ywave620:src-stream-improve -> nodejs:main
Labels     c++, lib / src, commit-queue-squash
Commits    2
 - src,stream: improve DoWrite() and Write()
 - src,stream: improve DoWrite() and Write()
Committers 1
 - rogertyang 
PR-URL: https://github.com/nodejs/node/pull/44434
Reviewed-By: Matteo Collina 
Reviewed-By: Stephen Belanger 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44434
Reviewed-By: Matteo Collina 
Reviewed-By: Stephen Belanger 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - src,stream: improve DoWrite() and Write()
   ⚠  - src,stream: improve DoWrite() and Write()
   ℹ  This PR was created on Mon, 29 Aug 2022 11:02:42 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/44434#pullrequestreview-1091912155
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/44434#pullrequestreview-1091998538
   ⚠  GitHub cannot link the author of 'src,stream: improve DoWrite() and Write()' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ⚠  GitHub cannot link the author of 'src,stream: improve DoWrite() and Write()' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-10-06T16:58:57Z: https://ci.nodejs.org/job/node-test-pull-request/47105/
- Querying data for job/node-test-pull-request/47105/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3205357558

@mcollina
Copy link
Member

mcollina commented Oct 7, 2022

@ywave620 could you please make sure that the email you use to commit is linked to your github account? Our tooling for landing PRs is failing.

Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork

@ywave620
Copy link
Contributor Author

ywave620 commented Oct 8, 2022

@mcollina Done :). Please try again.

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 8, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 8, 2022
@nodejs-github-bot
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/44434
✔  Done loading data for nodejs/node/pull/44434
----------------------------------- PR info ------------------------------------
Title      src,stream: improve DoWrite() and Write() (#44434)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     ywave620:src-stream-improve -> nodejs:main
Labels     c++, lib / src, commit-queue-squash
Commits    2
 - src,stream: improve DoWrite() and Write()
 - src,stream: improve DoWrite() and Write()
Committers 1
 - rogertyang 
PR-URL: https://github.com/nodejs/node/pull/44434
Reviewed-By: Matteo Collina 
Reviewed-By: Stephen Belanger 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44434
Reviewed-By: Matteo Collina 
Reviewed-By: Stephen Belanger 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - src,stream: improve DoWrite() and Write()
   ⚠  - src,stream: improve DoWrite() and Write()
   ℹ  This PR was created on Mon, 29 Aug 2022 11:02:42 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/44434#pullrequestreview-1091912155
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/44434#pullrequestreview-1091998538
   ⚠  GitHub cannot link the author of 'src,stream: improve DoWrite() and Write()' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-10-07T14:11:45Z: https://ci.nodejs.org/job/node-test-pull-request/47105/
- Querying data for job/node-test-pull-request/47105/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3209456339

@ywave620
Copy link
Contributor Author

ywave620 commented Oct 8, 2022

@mcollina Sorry. It turns out that the email used in the latest commit is in bad format, I need to rewrite the author metadata.

@ywave620
Copy link
Contributor Author

ywave620 commented Oct 8, 2022

@mcollina No substantial change at all, only the commit email is modified, shall we skip the CI phase?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 9, 2022
@mcollina
Copy link
Member

mcollina commented Oct 9, 2022

@mcollina No substantial change at all, only the commit email is modified, shall we skip the CI phase?

Our automated landing will complain if there are no CI run for the commit.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2022
@nodejs-github-bot
Copy link
Contributor

@ywave620
Copy link
Contributor Author

@mcollina CI done. Please help landing this :)

@mcollina mcollina added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 10, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2022
@nodejs-github-bot nodejs-github-bot merged commit 78d280a into nodejs:main Oct 10, 2022
52 checks passed
@nodejs-github-bot
Copy link
Contributor

Landed in 78d280a

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
Clarify StreamResource::DoWrite() interface definition.
Fix error-prone Http2Stream::DoWrite().
Change StreamBase::Write() to allow caller to avoid
inefficient write behavior.

PR-URL: #44434
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants