PDS pipethrough optimizations #2770

matthieusieben · 2024-08-31T21:11:06Z

No description provided.

matthieusieben · 2024-09-06T21:36:19Z

Still need to make sure tests pass and add some more

matthieusieben · 2024-09-06T21:39:28Z

packages/internal/fetch-node/src/ssrf.ts

@@ -43,15 +43,6 @@ export function ssrfFetchWrap<C = FetchContext>({
    }

    if (url.protocol === 'http:' || url.protocol === 'https:') {
-      // @ts-expect-error non-standard option
-      if (request.dispatcher) {


dispatcher was never exposed on Request

matthieusieben · 2024-09-06T21:40:11Z

packages/internal/fetch-node/src/ssrf.ts


-      if (allowUnknownTld !== true && !isValidDomain(url.hostname)) {


Will be performed through the lookup fn

…ations

devinivy · 2024-09-12T14:43:30Z

packages/common-web/src/did-doc.ts

+    const url = new URL(urlStr)
+
+    if (!url.hostname) {
+      return undefined
+    }


Is it possible for a url to start with http:// or https:// but successfully parse and not have a hostname? I wonder if we can use something that doesn't require throwing, e.g.

if (!URL.canParse(urlStr)) { return undefined } return urlStr

devinivy · 2024-09-12T15:05:49Z

packages/oauth/oauth-provider/src/lib/http/stream.ts

@@ -73,9 +47,9 @@ export async function parseStream(
  )

  if (!parser) {
-    throw createHttpError(400, 'Unsupported content-type')
+    throw new TypeError(`Unsupported content-type: ${type.mime}`)


I assume there's an error handler somewhere that needs to distinguish this TypeError, which should turn into a 415, from a TypeError coming from the runtime (e.g. null.x), which should turn into a 500. How would it distinguish them?

Handled here:

atproto/packages/oauth/oauth-provider/src/lib/http/request.ts

Lines 24 to 28 in 3368726

try {

return await parseStream(readable, req.headers['content-type'], allow)

} catch (err) {

throw createHttpError(400, err, { expose: err instanceof TypeError })

}

The idea is that parseStream is generic (does not know if its parsing a request, a response, a file on disk, etc.) so the responsibility to convert the error falls onto the caller.

As to distinguishing null pointer error, my hypothesis is that when using TypeScript, TypeError are more likely invalid input errors (e.g. new URL('ff')) rather than actual null pointer errors.

I could use a custom Error class for this particular purpose (because I don't want to throw something that is explicitely either a 415 or a 500). I just find TypeError to be handy and consistent with the way some native function work.

dholms · 2024-09-13T00:04:34Z

packages/common/src/streams.ts

  }
-  return new Uint8Array(Buffer.concat(bufs))
+  // Note Buffer is a subclass of Uint8Array
+  return Buffer.concat(chunks, totalLength)


any reason for changing the return type here?

i've been bit by subtle differences between Buffers & Uint8Arrays before & kinda prefer using just one or the other in most of our stuff is possible. or at least differentiating between the two and saying "bytes" for Uint8Arrays and "buffer" for Buffers

Buffer is a sub-class of Uint8Array. So it kinda makes no sense to wrap a Buffer within an Uint8Array since we loose functionalities & we create two objects instead of one.
Since this function is only used in the pipethrough code, I will rename the fn.

Note that common-web already has a streamToBuffer (that actually performs a AsyncIterable<Uint8Array> to Uint8Array). I think it's best if we don't change that function's name so I named this one to streamToNodeBuffer ...

dholms · 2024-09-13T00:10:09Z

packages/xrpc-server/src/server.ts

-            })
-          }
+        if (isHandlerPipeThroughBuffer(outputUnvalidated)) {
+          setHeaders(res, outputUnvalidated)


dholms · 2024-09-13T00:13:03Z

packages/xrpc-server/src/server.ts

-    }
-    if (!Object.values(v['headers']).every(isString)) {
-      return false
+const createValidator =


I think we avoided zod here because it can be kinda heavy duty for validation of stuff that is often invalid. I know it used to handle this by throwing and catching errors which can be rough on Node so we tried to keep it out of the happy/critical path of our server stuff.

But I also have some vague memory that they fixed this & made it faster? Any idea? Or any idea how these compare?

You are right, validating here is way overkill. Since we know the input is of type HandlerOutput, all we need is to discriminate based on that. We can even make some of the checks we currently do disappear !

dholms · 2024-09-13T00:14:09Z

packages/xrpc-server/src/server.ts

+const createValidator =
+  <T>(schema: ZodSchema<T>) =>
+  (v: unknown): v is T =>
+    schema.safeParse(v).success


we have a helper function for this that we use in most places in common-web/check

check.is(v, schema)

Added a check.create(schema) as the semantics I needed are a bit different

dholms · 2024-09-13T00:17:31Z

packages/xrpc-server/src/util.ts

-  return nsid.endsWith('/') ? nsid.slice(0, -1) : nsid // trim trailing slash
+  // /!\ Hot path
+
+  const originalUrl = ('originalUrl' in req && req.originalUrl) || req.url


could we rewrite as ternary? may be a bit easier to read on first glance

dholms · 2024-09-13T00:20:23Z

packages/pds/src/read-after-write/util.ts

@@ -23,42 +30,52 @@ export const getLocalLag = (local: LocalRecords): number | undefined => {
  return Date.now() - new Date(oldest).getTime()
 }

-export const handleReadAfterWrite = async <T>(
+export const pipethroughReadAfterWrite = async <T>(


dig this fn a lot

dholms · 2024-09-13T00:23:20Z

packages/pds/src/context.ts

+    const safeAgent = new undici.Agent({
+      allowH2: cfg.fetch.allowHTTP2, // This is experimental
+      headersTimeout: cfg.fetch.headersTimeout,
+      maxResponseSize: cfg.fetch.maxResponseSize,


are we cool re-using these config values for both the safeFetch & safeAgent?

only issue I could think of is if we have different tradeoffs on maxResponseSize

dholms · 2024-09-13T00:24:17Z

packages/pds/src/context.ts

@@ -256,12 +263,36 @@ export class AppContext {
      appviewCdnUrlPattern: cfg.bskyAppView?.cdnUrlPattern,
    })

+    const safeAgent = new undici.Agent({


I almost want to call this proxyAgent or safeProxyAgent or something similar to give the sense of what it's for since it's only use there

packages/pds/src/pipethrough.ts

dholms · 2024-09-13T00:52:06Z

packages/pds/src/pipethrough.ts

-  reqInit: RequestInit,
-): Promise<Response> => {
-  let res: Response
+type Accept = [name: string, flags: { q: string }]


thoughts on

type Accept = { name: string q: string }

instead of the array syntax? seems more clear & type-safe

The idea of this code is to be agnostic of the accept-* header being negotiated.

Consider for example:

Accept-Patch: text/example;charset=utf-8, text/example

In that case, the Accept type could be generalized as such:

type Accept = [name: string, flags: Record<string, string>]

When there is an obvious case for generalization, I like to write generic code. I guess this boils down to coding style preferences.

I can make this code more straight forward if you prefer or if you think this is too much YAGNI ?

What type safety difference are you thinking of ?

dholms

left some small tweaks/questions, but broadstrokes this looks awesome 💪 super stoked to get this in

…ations

matthieusieben force-pushed the msieben/micro-optimizations branch 4 times, most recently from 7d9350c to f629fc7 Compare September 2, 2024 18:39

Vincentroland approved these changes Sep 5, 2024

View reviewed changes

matthieusieben force-pushed the msieben/micro-optimizations branch from 956c637 to 343dc79 Compare September 6, 2024 14:28

matthieusieben changed the title ~~Micro optimizations in PDS~~ PDS pipethrough optimizations Sep 6, 2024

matthieusieben added 8 commits September 6, 2024 23:34

Micro optimization in request proxying

d5cf39d

Request NSID parsing optimization

865962c

DID document parsing optimization

99202fe

remove un-necessary call to next()

d6636c0

Allow HandlerPipeThrough to be used with streams

d8e2567

Refactor pipethrough to work with streams

eacbeb3

Expose "unicastLookup" DNS lookup and "isUnicastIp" utilities

83c569c

Use a hardened, HTTP2 compatible, client to perform proxied requests

10da838

matthieusieben force-pushed the msieben/micro-optimizations branch from 904663b to 10da838 Compare September 6, 2024 21:34

matthieusieben requested a review from devinivy September 6, 2024 21:36

changeset

a681c7c

matthieusieben commented Sep 6, 2024

View reviewed changes

tidy

5ff6633

matthieusieben commented Sep 7, 2024

View reviewed changes

matthieusieben added 8 commits September 9, 2024 22:20

Properly handle compressed streams

f94195c

tidy

c6b345b

update @types/node

b008633

refactor

07ebbc5

Merge remote-tracking branch 'origin/main' into msieben/micro-optimiz…

198528c

…ations

Improved error management

73b0e31

Expose parseContentEncoding() util

964d8ea

use pipeline from nodejs

ee95a9e

matthieusieben added 3 commits September 12, 2024 15:21

increase highWaterMark

bb5fb3c

Merge remote-tracking branch 'origin/main' into msieben/micro-optimiz…

3368726

…ations

remove un-necessary type check

4d778a0

devinivy reviewed Sep 12, 2024

View reviewed changes

dholms reviewed Sep 13, 2024

View reviewed changes

packages/pds/src/pipethrough.ts Show resolved Hide resolved

dholms reviewed Sep 13, 2024

View reviewed changes

dholms approved these changes Sep 13, 2024

View reviewed changes

matthieusieben added 13 commits September 13, 2024 09:25

Use undici.request where more relevant

546b51e

Improve soc in fetch utils

4788c1c

feedback

1b02b58

fidy

09dfc94

tidy

3adbf20

test refactor

d990c44

safer fetch

56f983a

Merge remote-tracking branch 'origin/main' into msieben/micro-optimiz…

30d6ec3

…ations

changeset

05baefc

expose and re-use extractUrl util

cd6a6dc

small optimizations

09143dd

tidy

19d2e24

optimization

f1639a5

matthieusieben force-pushed the msieben/micro-optimizations branch from 205bbae to f1639a5 Compare September 16, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PDS pipethrough optimizations #2770

PDS pipethrough optimizations #2770

matthieusieben commented Aug 31, 2024

matthieusieben commented Sep 6, 2024 •

edited

Loading

matthieusieben Sep 6, 2024

matthieusieben Sep 6, 2024

devinivy Sep 12, 2024 •

edited

Loading

matthieusieben Sep 13, 2024

devinivy Sep 12, 2024

matthieusieben Sep 13, 2024 •

edited

Loading

matthieusieben Sep 16, 2024

matthieusieben Sep 16, 2024

dholms Sep 13, 2024

matthieusieben Sep 13, 2024

matthieusieben Sep 13, 2024

dholms Sep 13, 2024

dholms Sep 13, 2024

matthieusieben Sep 13, 2024

dholms Sep 13, 2024

matthieusieben Sep 13, 2024

dholms Sep 13, 2024

dholms Sep 13, 2024

dholms Sep 13, 2024

dholms Sep 13, 2024

dholms Sep 13, 2024

matthieusieben Sep 13, 2024 •

edited

Loading

dholms left a comment


		if (allowUnknownTld !== true && !isValidDomain(url.hostname)) {

	try {
	return await parseStream(readable, req.headers['content-type'], allow)
	} catch (err) {
	throw createHttpError(400, err, { expose: err instanceof TypeError })
	}

PDS pipethrough optimizations #2770

Are you sure you want to change the base?

PDS pipethrough optimizations #2770

Conversation

matthieusieben commented Aug 31, 2024

matthieusieben commented Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieusieben Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieusieben Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

dholms left a comment

Choose a reason for hiding this comment

matthieusieben commented Sep 6, 2024 •

edited

Loading

devinivy Sep 12, 2024 •

edited

Loading

matthieusieben Sep 13, 2024 •

edited

Loading

matthieusieben Sep 13, 2024 •

edited

Loading