Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

quic: implement sendFD() support #150

Closed
wants to merge 7 commits into from
Closed

quic: implement sendFD() support #150

wants to merge 7 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 3, 2019

Fun fact: The first commit was written in March 2018, when I started working on something cool but never got anywhere with it … finally something to use it for :)

This is mostly based on the HTTP/2 implementation for the same thing.

TODO:

  • Docs
  • Tests for error conditions
  • Figure out why .close() is not enough to shut down the test
  • sendFile() with a file path
  • Direct support for FileHandle as returned from fs.promises.open (this goes for HTTP/2 too)

@jasnell
Copy link
Member

jasnell commented Oct 3, 2019

Oh very nice!

const fixtures = require('../common/fixtures');
const key = fixtures.readKey('agent1-key.pem', 'binary');
const cert = fixtures.readKey('agent1-cert.pem', 'binary');
const ca = fixtures.readKey('ca1-cert.pem', 'binary');
Copy link
Member

Choose a reason for hiding this comment

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

todo for later... it's so common for us to read all three of these together we can simplify things a bit by adding an additional fixture method... e.g.

something like...

const { key, cert, ca } = fixtures.readTLSKey('agent1-key.pem',
											  'agent1-cert.pem',
											  'ca1-cert.pem');

@addaleax
Copy link
Member Author

addaleax commented Oct 7, 2019

HTTP/2 PR for FileHandle support: nodejs/node#29876

I’d implement that here depending on whether it lands in HTTP/2.

@jasnell
Copy link
Member

jasnell commented Oct 9, 2019

This will likely need a rebase

@addaleax addaleax marked this pull request as ready for review October 9, 2019 23:01
@addaleax
Copy link
Member Author

addaleax commented Oct 9, 2019

Rebased. This PR is ready now, and contains two more upstream commits that are here to prevent conflicts at the next rebase.

@addaleax addaleax changed the title [WIP] quic: implement sendFD() support quic: implement sendFD() support Oct 9, 2019
Fishrock123 and others added 5 commits October 10, 2019 01:12
This adds long-requested methods for asynchronously interacting and
iterating through directory entries by using `uv_fs_opendir`,
`uv_fs_readdir`, and `uv_fs_closedir`.

`fs.opendir()` and friends return an `fs.Dir`, which contains methods
for doing reads and cleanup. `fs.Dir` also has the async iterator
symbol exposed.

The `read()` method and friends only return `fs.Dirent`s for this API.
Having a entry type or doing a `stat` call is deemed to be necessary in
the majority of cases, so just returning dirents seems like the logical
choice for a new api.

Reading when there are no more entries returns `null` instead of a
dirent. However the async iterator hides that (and does automatic
cleanup).

The code lives in separate files from the rest of fs, this is done
partially to prevent over-pollution of those (already very large)
files, but also in the case of js allows loading into `fsPromises`.

Due to async_hooks, this introduces a new handle type of `DIRHANDLE`.

This PR does not attempt to make complete optimization of
this feature. Notable future improvements include:
- Moving promise work into C++ land like FileHandle.
- Possibly adding `readv()` to do multi-entry directory reads.
- Aliasing `fs.readdir` to `fs.scandir` and doing a deprecation.

Refs: nodejs/node-v0.x-archive#388
Refs: nodejs/node#583
Refs: libuv/libuv#2057

PR-URL: nodejs/node#29349
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
This seems to make sense if we want to promote the use
of `fs.promises`, although it’s not strictly necessary.

PR-URL: nodejs/node#29876
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
doc/api/quic.md Outdated Show resolved Hide resolved
doc/api/quic.md Outdated Show resolved Hide resolved
addaleax and others added 2 commits October 10, 2019 23:15
Co-Authored-By: James M Snell <jasnell@gmail.com>
Co-Authored-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Oct 11, 2019
PR-URL: #150
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Oct 11, 2019
Fixes: #75
PR-URL: #150
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

Landed in 6b97000...a19682e

@addaleax addaleax closed this Oct 11, 2019
@addaleax addaleax deleted the quic-send-fd branch October 11, 2019 20:10
addaleax added a commit that referenced this pull request Dec 11, 2019
PR-URL: #150
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Dec 11, 2019
Fixes: #75
PR-URL: #150
Reviewed-By: James M Snell <jasnell@gmail.com>
sam-github pushed a commit to sam-github/node that referenced this pull request Dec 13, 2019
PR-URL: nodejs/quic#150
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
PR-URL: nodejs#150
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
Fixes: nodejs#75
PR-URL: nodejs#150
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
Fixes: nodejs#75
PR-URL: nodejs#150
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit to jasnell/quic that referenced this pull request Feb 3, 2020
PR-URL: nodejs#150
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Feb 13, 2020
PR-URL: #150
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit to jasnell/node that referenced this pull request Feb 19, 2020
Originally landed in the nodejs/quic repo and used there for file
sending.

Original review:

```
  PR-URL: nodejs/quic#150
  Reviewed-By: James M Snell <jasnell@gmail.com>
```
jasnell pushed a commit to nodejs/node that referenced this pull request Feb 26, 2020
Originally landed in the nodejs/quic repo and used there for file
sending.

Original review:

```
  PR-URL: nodejs/quic#150
  Reviewed-By: James M Snell <jasnell@gmail.com>
```

PR-URL: #31869
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Feb 27, 2020
Originally landed in the nodejs/quic repo and used there for file
sending.

Original review:

```
  PR-URL: nodejs/quic#150
  Reviewed-By: James M Snell <jasnell@gmail.com>
```

PR-URL: #31869
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Mar 15, 2020
Originally landed in the nodejs/quic repo and used there for file
sending.

Original review:

```
  PR-URL: nodejs/quic#150
  Reviewed-By: James M Snell <jasnell@gmail.com>
```

PR-URL: #31869
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Mar 17, 2020
Originally landed in the nodejs/quic repo and used there for file
sending.

Original review:

```
  PR-URL: nodejs/quic#150
  Reviewed-By: James M Snell <jasnell@gmail.com>
```

PR-URL: #31869
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this pull request Mar 30, 2020
Originally landed in the nodejs/quic repo and used there for file
sending.

Original review:

```
  PR-URL: nodejs/quic#150
  Reviewed-By: James M Snell <jasnell@gmail.com>
```

PR-URL: #31869
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants