Add unix socket #158

cecton · 2021-11-17T18:21:27Z

Linking this issue here as these traits could be moved to tokio-util: ~~tokio-rs/tokio#4244~~ tokio-rs/tokio#4385

It's working!

[7] [16:11:40] ~ > curl -v --unix-socket /tmp/router.sock http://localhost:4000/graphql -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0' -H 'Accept: */*' -H 'Accept-Language: en-US,en;q=0.5' --compressed -H 'content-type: application/json' -H 'Origin: https://studio.apollographql.com' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Sec-Fetch-Dest: empty' -H 'Sec-Fetch-Mode: cors' -H 'Sec-Fetch-Site: cross-site' -H 'Sec-GPC: 1' --data-raw '{"query":"{topProducts{upc name __typename}}"}'
*   Trying /tmp/router.sock:0...
* Connected to localhost (/tmp/router.sock) port 4000 (#0)
> POST /graphql HTTP/1.1
> Host: localhost:4000
> Accept-Encoding: deflate, gzip, br, zstd
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0
> Accept: */*
> Accept-Language: en-US,en;q=0.5
> content-type: application/json
> Origin: https://studio.apollographql.com
> DNT: 1
> Connection: keep-alive
> Sec-Fetch-Dest: empty
> Sec-Fetch-Mode: cors
> Sec-Fetch-Site: cross-site
> Sec-GPC: 1
> Content-Length: 46
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< access-control-expose-headers: content-type
< access-control-allow-origin: https://studio.apollographql.com
< transfer-encoding: chunked
< date: Thu, 18 Nov 2021 16:11:47 GMT
< 
* Connection #0 to host localhost left intact
{"data":{"topProducts":[{"upc":"1","name":"Table","__typename":"Product"},{"upc":"2","name":"Couch","__typename":"Product"},{"upc":"3","name":"Chair","__typename":"Product"}]}}⏎

Fixes #154

o0Ignition0o · 2021-11-18T16:24:40Z

This looks super cool! is there any issue / context this pr refers to ?

EDIT: my bad, i didn't notice #154 below

cecton · 2021-11-18T16:29:53Z

This looks super cool! is there any issue / context this pr refers to ?

EDIT: my bad, i didn't notice #154 below

I added it after I requested your review lol

cecton · 2021-11-18T16:31:25Z

This looks super cool!

Actually it's terrible xD There should be a way to get a listener that is either a unix socket or a tcp listener. I made a PR to add that to tokio-util 🤞

o0Ignition0o

Wow this is great work! 🎉 the pull request reads incredibly well!

+1 for me, with only one question / comment.

Pre-approving this, although we might want to wait a bit and see if the tokio team wants to accept the listener trait.

crates/apollo-router/src/configuration/mod.rs

cecton · 2021-11-19T08:21:11Z

Pre-approving this, although we might want to wait a bit and see if the tokio team wants to accept the listener trait.

Yes I would too but I expect this to take some time. The current implementation is quite easily replaceable so I wouldn't mind merging this for now.

Geal · 2021-11-19T08:56:19Z

crates/apollo-router/src/http_server_factory.rs

+pub(crate) type BoxAsyncReadWrite = Box<dyn AsyncReadWrite + Send + Unpin + 'static>;
+
+impl AsyncReadWrite for TcpStream {
+    fn set_nodelay(&self, nodelay: bool) -> io::Result<()> {


this should be done right at socket creation, there's no need to add an empty method for UnixStream

Outdated, I do not touch that part anymore

Geal · 2021-11-19T09:01:16Z

crates/apollo-router/src/http_server_factory.rs

+    fn set_nodelay(&self, nodelay: bool) -> io::Result<()>;
+}
+
+pub(crate) type BoxAsyncReadWrite = Box<dyn AsyncReadWrite + Send + Unpin + 'static>;


it's not a good idea to have a Box<dyn> to hold either a TcpStream or UnixStream. It is useful when we're handling both kinds of objects at runtime, but here we're always creating one kind of object (we won't run in TCP and Unix socket modes at the same time), so we should not bear the cost of adding an allocation in the hot path for that.
AsyncRead has one method, AsyncWrite has 5 methods, so it's still reasonable to make an enum here.

Fixed with new impl

cecton · 2022-01-28T15:41:05Z

This PR is ready for review again

apollo-router/src/http_server_factory.rs

Co-authored-by: Gary Pennington <[email protected]>

code changed completely

apollo-router/Cargo.toml

Co-authored-by: Coenen Benjamin <[email protected]>

cecton added 4 commits November 17, 2021 19:21

Unix socket

af7ad42

CLEANUP

218c0d7

Missing feature gate for windows

c083323

clippy fixes

617c6d8

cecton self-assigned this Nov 18, 2021

cecton added 5 commits November 18, 2021 15:24

CLEANUP

c911e9b

Simplify type complexity

bde4185

Cleanup socket file at shutdown

884f3d6

Fix incorrect display for server URL

5d39810

Add config file in examples

bb45d19

cecton marked this pull request as ready for review November 18, 2021 16:21

cecton requested review from Geal, BrynCooke and o0Ignition0o November 18, 2021 16:21

o0Ignition0o approved these changes Nov 18, 2021

View reviewed changes

crates/apollo-router/src/configuration/mod.rs Outdated Show resolved Hide resolved

Geal previously requested changes Nov 19, 2021

View reviewed changes

cecton marked this pull request as draft December 1, 2021 13:29

cecton added the size/large label Dec 1, 2021

cecton added 2 commits January 12, 2022 17:26

Update with suggestion from tokio folks

575d2d7

Compatibility with Windows

52bac2f

abernix removed the size/large label Jan 25, 2022

cecton added 5 commits January 28, 2022 11:05

Revert to origin/main

345bb34

Update to tokio 1.16

6d00f94

Unix socket

c18e132

Cleanup socket file at shutdown

bae03ba

Windows compatibility

4a1ec0d

cecton added 4 commits January 28, 2022 12:25

Fixing tests

ab45ad2

Compliance

84ab22b

Fix deactivated test

3ee56be

Use tempfile to create unix sockets

e62292e

cecton marked this pull request as ready for review January 28, 2022 15:40

cecton requested review from Geal, garypen and o0Ignition0o January 28, 2022 15:40

garypen approved these changes Jan 31, 2022

View reviewed changes

apollo-router/src/http_server_factory.rs Outdated Show resolved Hide resolved

Update apollo-router/src/http_server_factory.rs

1f69137

Co-authored-by: Gary Pennington <[email protected]>

bnjjj reviewed Feb 1, 2022

View reviewed changes

apollo-router/Cargo.toml Show resolved Hide resolved

Update apollo-router/Cargo.toml

aeb8891

Co-authored-by: Coenen Benjamin <[email protected]>

cecton merged commit 948dbc6 into main Feb 2, 2022

cecton deleted the cecton-unix-socket branch February 2, 2022 07:00

garypen mentioned this pull request Feb 11, 2022

Startup output says "Listening on http://http://" #447

Closed

tinnou pushed a commit to Netflix-Skunkworks/router that referenced this pull request Oct 16, 2023

release: [email protected] (apollographql#158)

171dc67

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unix socket #158

Add unix socket #158

cecton commented Nov 17, 2021 •

edited

Loading

o0Ignition0o commented Nov 18, 2021 •

edited

Loading

cecton commented Nov 18, 2021

cecton commented Nov 18, 2021

o0Ignition0o left a comment

cecton commented Nov 19, 2021

Geal Nov 19, 2021

cecton Jan 28, 2022

Geal Nov 19, 2021

cecton Jan 28, 2022

cecton commented Jan 28, 2022

Add unix socket #158

Add unix socket #158

Conversation

cecton commented Nov 17, 2021 • edited Loading

o0Ignition0o commented Nov 18, 2021 • edited Loading

cecton commented Nov 18, 2021

cecton commented Nov 18, 2021

o0Ignition0o left a comment

Choose a reason for hiding this comment

cecton commented Nov 19, 2021

Geal Nov 19, 2021

Choose a reason for hiding this comment

cecton Jan 28, 2022

Choose a reason for hiding this comment

Geal Nov 19, 2021

Choose a reason for hiding this comment

cecton Jan 28, 2022

Choose a reason for hiding this comment

cecton commented Jan 28, 2022

cecton commented Nov 17, 2021 •

edited

Loading

o0Ignition0o commented Nov 18, 2021 •

edited

Loading