fix cdn race condition #424

silesky · 2022-04-08T01:32:04Z

I'm still evaluating if it's safe to remove the setGlobalCDNUrl call from brower-umd.ts and standalone.ts (if anyone has any thoughts / I'm interested!).

silesky · 2022-04-08T01:37:24Z

example/pages/index.tsx

-  const [settings, setSettings] = useState<AnalyticsSettings | undefined>(
-    undefined
-  )
+  const [writeKey, setWriteKey] = useLocalStorage<string>('__dev_writeKey', '')


This is a somewhat unrelated change that I'm sneaking in -- I got a bit tired of having the writeKey input not persist between refreshes / server restarts / etc.

silesky · 2022-04-08T01:44:07Z

src/plugins/ajs-destination/loader.ts

@@ -42,6 +39,7 @@ export async function loadIntegration(
 ): Promise<LegacyIntegration> {
  const pathName = normalizeName(name)
  const obfuscatedPathName = obfuscatePathName(pathName, obfuscate)
+  const path = getNextIntegrationsURL()


removing this method call from the global scope makes it easier to reason about, especially since it's a wrapper around a mutable global; we don't need to worry about stale references.

yordis · 2022-04-08T02:52:20Z

example/pages/index.tsx

+  const [writeKey, setWriteKey] = useLocalStorage<string>('__dev_writeKey', '')
+  const [settings, setSettings] = useState<
+    AnalyticsBrowserSettings | undefined
+  >({
+    writeKey,
+    cdnURL: 'https://cdn.segment.com',
+  })


Suggested change

const [writeKey, setWriteKey] = useLocalStorage<string>('__dev_writeKey', '')

const [settings, setSettings] = useState<

AnalyticsBrowserSettings | undefined

>({

writeKey,

cdnURL: 'https://cdn.segment.com',

})

const [writeKey, setWriteKey] = useLocalStorage<string>('__dev_writeKey', '')

const [settings, setSettings] = useState<

AnalyticsBrowserSettings | undefined

>({

writeKey: process.env.NEXT_PUBLIC_SEGMENT_WRITE_KEY,

cdnURL: process.env.NEXT_PUBLIC_SEGMENT_CDN_URL,

})

I would not like to teach people to do this ... it will cause issues like caching the wrong value or somebody blocking the localStorage ... you named it.

You do you, but I rather teach people to put such information in the env file since that is probably where people should be injecting such thing, and it will fix your issue

I didn't even consider that people might want to override cdnURL, but that's a good idea!

on the subject of writeKey -- on a regular application, I agree. However, this is a playground for debugging (run make dev if you haven't yet to see what I mean!) -- so there's a writeKey input field that allows people to swap out writeKeys at runtime. This just makes it easier for people to use their most recent one.

I didn't even consider that people might want to override cdnURL, but that's a good idea!

Well, we have to do a proxy to Segment to bypass AdBlocker ... so probably most companies that know a bit about infra would do that most of the time 😄

run make dev if you haven't yet to see what I mean!

I see I see, would call the directory "playground" because people are getting used to the idea of using examples directory to learn how to get up to speed with this ... which is my actually worry (not you, but others reading code out of context)

I didn't even consider that people might want to override cdnURL, but that's a good idea!

Well, we have to do a proxy to Segment to bypass AdBlocker ... so probably most companies that know a bit about infra would do that most of the time 😄

Yep, I just meant the few of us who use the playground (just me?) — I’d just seen it used to demo segment and develop locally directly against the default segment cdn. Anyway, now it’s overridable!

run make dev if you haven't yet to see what I mean!

I see I see, would call the directory "playground" because people are getting used to the idea of using examples directory to learn how to get up to speed with this ... which is my actually worry (not you, but others reading code out of context)

I think it’s meant to serve a duel role, but that’s a good point — it’s not typically what you think of as an ‘example’ in an examples repo!

yordis · 2022-04-08T02:53:25Z

src/lib/parse-cdn.ts

@@ -17,14 +17,17 @@ const getCDNUrlFromScriptTag = (): string | undefined => {
  return cdn
 }

+let _globalCDN: string | undefined // set globalCDN as in-memory singleton


src/lib/parse-cdn.ts

pooyaj · 2022-04-08T04:19:35Z

@silesky I think the purpose of the setGlobalCDNUrl in the standalone etc. is to avoid consistently hitting the getCDNUrlFromScriptTag when calling getCDN, and only using getCDNUrlFromScriptTag once ( because it is a relatively costly DOM scan ).

silesky · 2022-04-08T17:07:53Z

@silesky I think the purpose of the setGlobalCDNUrl in the standalone etc. is to avoid consistently hitting the getCDNUrlFromScriptTag when calling getCDN, and only using getCDNUrlFromScriptTag once ( because it is a relatively costly DOM scan ).

@pooyaj Ah, If that's true that it's a cache, do you mind if we get rid of it? It seems like an unneeded premature optimization, particularly for a function that's only called during a couple initialization steps. It would make things a lot cleaner and remove the possibility of race conditions (which shouldn't happen in practice, but you have to know about the lifecycle to discount that possibility).

We can do this in additional PRs FWIW

chrisradek

Looks great!

With regards to your question on if we need to setCdn for standalone/browser.umd, I think @pooyaj's explanation makes sense. If we can guarantee that we would only do the script tag lookup once with the changes you've made we can likely remove it - but I think to keep the scope of this PR focused I'd rather create a new issue or JIRA to track making that change/testing it.

chrisradek · 2022-04-08T19:34:46Z

src/__tests__/integration.test.ts

+        ],
+      })
+      expect(fetchCalls[0][0]).toContain(overriddenCDNUrl)
+      expect.assertions(3)


That's a neat trick 😄

silesky requested a review from chrisradek April 8, 2022 01:32

silesky commented Apr 8, 2022

View reviewed changes

silesky requested a review from pooyaj April 8, 2022 01:54

yordis reviewed Apr 8, 2022

View reviewed changes

src/lib/parse-cdn.ts Outdated Show resolved Hide resolved

silesky force-pushed the bug/fix-cdn-race-condition branch 3 times, most recently from 691267f to 2d8f259 Compare April 8, 2022 04:37

silesky requested a review from danieljackins April 8, 2022 17:00

silesky mentioned this pull request Apr 8, 2022

Allow cdn in AnalyticsSettings #362

Closed

silesky force-pushed the bug/fix-cdn-race-condition branch 3 times, most recently from 952b810 to c4c2f2c Compare April 8, 2022 19:40

chrisradek approved these changes Apr 8, 2022

View reviewed changes

silesky force-pushed the bug/fix-cdn-race-condition branch from c4c2f2c to 8c04016 Compare April 8, 2022 19:43

add integration test for cdn, update example, etc

1266c93

silesky force-pushed the bug/fix-cdn-race-condition branch from 8c04016 to 1266c93 Compare April 8, 2022 19:47

silesky merged commit 781e2d6 into master Apr 8, 2022

silesky deleted the bug/fix-cdn-race-condition branch April 8, 2022 19:58

adisbanda pushed a commit to juneHQ/june-analytics-next that referenced this pull request Jul 13, 2022

add integration test for cdn, update example, etc (segmentio#424)

64d73c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix cdn race condition #424

fix cdn race condition #424

silesky commented Apr 8, 2022 •

edited

silesky Apr 8, 2022 •

edited

danieljackins Apr 8, 2022

silesky Apr 8, 2022 •

edited

yordis Apr 8, 2022

silesky Apr 8, 2022 •

edited

yordis Apr 8, 2022

yordis Apr 8, 2022

silesky Apr 8, 2022 •

edited

silesky Apr 8, 2022

yordis Apr 8, 2022

pooyaj commented Apr 8, 2022

silesky commented Apr 8, 2022 •

edited

chrisradek left a comment

chrisradek Apr 8, 2022

fix cdn race condition #424

fix cdn race condition #424

Conversation

silesky commented Apr 8, 2022 • edited

silesky Apr 8, 2022 • edited

Choose a reason for hiding this comment

danieljackins Apr 8, 2022

Choose a reason for hiding this comment

silesky Apr 8, 2022 • edited

Choose a reason for hiding this comment

yordis Apr 8, 2022

Choose a reason for hiding this comment

silesky Apr 8, 2022 • edited

Choose a reason for hiding this comment

yordis Apr 8, 2022

Choose a reason for hiding this comment

yordis Apr 8, 2022

Choose a reason for hiding this comment

silesky Apr 8, 2022 • edited

Choose a reason for hiding this comment

silesky Apr 8, 2022

Choose a reason for hiding this comment

yordis Apr 8, 2022

Choose a reason for hiding this comment

pooyaj commented Apr 8, 2022

silesky commented Apr 8, 2022 • edited

chrisradek left a comment

Choose a reason for hiding this comment

chrisradek Apr 8, 2022

Choose a reason for hiding this comment

silesky commented Apr 8, 2022 •

edited

silesky Apr 8, 2022 •

edited

silesky Apr 8, 2022 •

edited

silesky Apr 8, 2022 •

edited

silesky Apr 8, 2022 •

edited

silesky commented Apr 8, 2022 •

edited