feat(devtools-proxy-support): initial shared implementation COMPASS-8072 #418
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
addaleax
changed the title
| while (this._reqLock) { | ||
| await this._reqLock; | ||
| } |
There was a problem hiding this comment.
Just curious why we are using while here: is there a possibility of a microtask happening between await resolving and the execution going to the beginning of the loop to check the _reqLock?
There was a problem hiding this comment.
is there a possibility of a microtask happening between await resolving and the execution going to the beginning of the loop to check the
_reqLock?
The await here does always wait at least one microtask queue iteration, and during that iteration other things can happen (such as other Promises being resolved and handlers for that resolution running)
Just curious why we are using while here
Yeah, it's to avoid a scenario where if .connect() gets called twice (or more) in a synchronous code block, both of those .connect() calls would await the same _reqLock Promise and then simultaneously proceed (and each of them create new, independent _reqLock promises, overwriting each other)
There was a problem hiding this comment.
Aha, got it, thanks for a detailed explanation with an example, it like made sense, but was still hard to wrap my head around the details of the flow without it
There was a problem hiding this comment.
Yeah ... it didn't feel worth pulling in an external library for this, but if you feel like that would be a good idea to make it more obvious what's happening here, I am not opposed to that at all (because it's not just you, I also had to give this a bit of thought to be confident that it's correct the way it is)
There was a problem hiding this comment.
I think it's good as is! If you want to add some sort of comment in the code for this (like this example you provided I think made it very clear for me what this is guarding against), it would be enough I think, but even without it, it's fine, if it raises questions at the very least it will be possible to track it to this conversation now 🙂
There was a problem hiding this comment.
Sure, added a small comment about this as well 👍
Co-authored-by: Sergey Petushkov <[email protected]>
|
|
||
| // The original version of this code was largely taken from | ||
| // https://github.com/mongodb-js/mongosh/blob/8e6962432397154941f593c847d8f774bfd49f1c/packages/import-node-fetch/src/index.ts | ||
| async function importNodeFetch(): Promise<typeof fetch> { |
A few things small that might be helpful for reviewing this PR:
Some potentially helpful reading:
CONNECTproxiesPAC (proxy auto-config) filesAgentis a connection/socket factory)proxy-agentpackage READMEThis PR will replace:
@mongosh/import-node-fetchin mongosh@mongodb-js/ssh-tunnelin Compass (split into two things,SSHAgentandSocks5Server)I'd recommend starting with
proxy-options.tsandagent.tsand their associated test files, then work onwards from there.