Skip to content

build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot#34454

Closed
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:fix-quic
Closed

build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot#34454
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:fix-quic

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jul 21, 2020 •
edited
Loading

build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot

Otherwise the build would fail with
./configure --experimental-quic --ninja as the list of per-Environment
values would not match and the code cache builder would not generate
code cache for the quic JS sources. This is more or less a band-aid -
a proper fix would be to aggregate these flags into something
that can be included by all these different binary targets.
See #31074.

Fixes: #34435

Revert "src: refactor TimerWrap lifetime management"

This reverts commit 874460a.

@addaleax I have no idea why but it seems the TimerWrap lifetime management refactoring 874460a is also causing the build to fail a bunch of quic tests, crashing with

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x40)
    frame #0: 0x000000010019c3c8 node`node::TimerWrapHandle::Stop() [inlined] node::TimerWrap::Stop(this=0x0000000000000000) at timer_wrap.cc:16:19 [opt]
   13  	}
   14
   15  	void TimerWrap::Stop() {
-> 16  	  if (timer_.data == nullptr) return;
   17  	  uv_timer_stop(&timer_);
   18  	}
   19

So I revereted it as well to make the tests pass

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@joyeecheung joyeecheung requested a review from a team July 21, 2020 01:43
@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. labels Jul 21, 2020
@joyeecheung joyeecheung added the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Jul 21, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 21, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/32433/

@joyeecheung
Copy link
Member Author

joyeecheung commented Jul 21, 2020 •
edited
Loading

Custom build with --experimental-quic (pending) https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/15490/

@jasnell
Copy link
Member

jasnell commented Jul 21, 2020 •
edited
Loading

We should try to fix those reverts before landing this so we don't have to revert them. I'll be able to take a look tomorrow

addaleax added a commit to addaleax/node that referenced this pull request Jul 21, 2020
@addaleax
src: guard against nullptr deref in TimerWrapHandle::Stop
4d35330 
Refs: nodejs#34454
@addaleax addaleax mentioned this pull request Jul 21, 2020
Closed
2 tasks
@addaleax
Copy link
Member

addaleax commented Jul 21, 2020

Not sure why this PR would expose something like this, but here’s a fix: #34460

@jasnell
Copy link
Member

jasnell commented Jul 21, 2020

Once #34460 lands, the revert shouldn't be necessary and this otherwise LGTM

addaleax added a commit that referenced this pull request Jul 21, 2020
@addaleax
src: guard against nullptr deref in TimerWrapHandle::Stop
feb93c4 
Refs: #34454

PR-URL: #34460
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@github-actions github-actions bot mentioned this pull request Jul 22, 2020
Open
5 tasks
@joyeecheung
build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot
e4d50ab 
Otherwise the build would fail with
`./configure --experimental-quic --ninja` as the list of per-Environment
values would not match and the code cache builder would not generate
code cache for the quic JS sources. This is more or less a band-aid -
a proper fix would be to aggregate these flags into something
that can be included by all these different binary targets.
See nodejs#31074.
@joyeecheung
Copy link
Member Author

joyeecheung commented Jul 22, 2020

Rebased. @nodejs/quic PTAL, thanks!

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 22, 2020 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32466/ (:green_heart:)

@joyeecheung joyeecheung changed the title quic: fix --experimental-quic build build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot Jul 22, 2020
@joyeecheung
Copy link
Member Author

joyeecheung commented Jul 22, 2020 •
edited by addaleax
Loading

Custom build with --experimental-quic (pending) https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/15520/ (:green_heart:)

@gengjiawen
Copy link
Member

gengjiawen commented Jul 22, 2020

Fast track ?

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 72 hours to land. labels Jul 22, 2020
gengjiawen pushed a commit that referenced this pull request Jul 22, 2020
@joyeecheung @gengjiawen
build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot
f4f191b 
Otherwise the build would fail with
`./configure --experimental-quic --ninja` as the list of per-Environment
values would not match and the code cache builder would not generate
code cache for the quic JS sources. This is more or less a band-aid -
a proper fix would be to aggregate these flags into something
that can be included by all these different binary targets.
See #31074.

PR-URL: #34454
Fixes: #34435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@gengjiawen
Copy link
Member

gengjiawen commented Jul 22, 2020

Landed in f4f191b

@gengjiawen gengjiawen closed this Jul 22, 2020
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
@addaleax @cjihrig
src: guard against nullptr deref in TimerWrapHandle::Stop
fe69e1e 
Refs: #34454

PR-URL: #34460
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
@joyeecheung @cjihrig
build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot
29309f6 
Otherwise the build would fail with
`./configure --experimental-quic --ninja` as the list of per-Environment
values would not match and the code cache builder would not generate
code cache for the quic JS sources. This is more or less a band-aid -
a proper fix would be to aggregate these flags into something
that can be included by all these different binary targets.
See #31074.

PR-URL: #34454
Fixes: #34435
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
This was referenced Jul 23, 2020
Open
Open
Open
Open
Open
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 27, 2020

Adding don't land labels for 10.x - 14.x as this is related to quic

@github-actions github-actions bot mentioned this pull request Jul 28, 2020
Open
3 tasks
targos pushed a commit that referenced this pull request May 1, 2021
@addaleax @targos
src: guard against nullptr deref in TimerWrapHandle::Stop
b1d5160 
Refs: #34454

PR-URL: #34460
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 72 hours to land. lib / src Issues and PRs related to general changes in the lib or src directory. quic Issues and PRs related to the QUIC implementation / HTTP/3.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ninja build failed when build with quic

6 participants

@joyeecheung @nodejs-github-bot @jasnell @addaleax @gengjiawen @MylesBorins