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.

Sign up for GitHub

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

Jump to bottom

build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot #34454

Closed
wants to merge 1 commit into from
Closed

build: define NODE_EXPERIMENTAL_QUIC in mkcodecache and node_mksnapshot #34454

wants to merge 1 commit into from

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

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

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

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

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 48 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

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

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

@gengjiawen gengjiawen gengjiawen approved these changes

@jasnell jasnell jasnell 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 48 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