Skip to content
This repository has been archived by the owner on Feb 13, 2021. It is now read-only.

fixed issue regarding concurrent requests to dns #6

Closed
wants to merge 1 commit into from

Conversation

esnunes
Copy link

@esnunes esnunes commented Jul 21, 2015

No description provided.

@davglass
Copy link
Contributor

Nice catch!

@davglass
Copy link
Contributor

@drewfish @sylvio

@yahoocla
Copy link

CLA is valid!

list.forEach(function (cb) { cb(err); });
delete callbackList[key];
} else {
cache.set('lookup_' + domain + '_' + family, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use the key variable here too? If we change the way the key is constructed it'd be nice to only do it in one place.

@drewfish
Copy link
Contributor

Good stuff.

When calling each callback (list.forEach(function (cb) { cb(...); })) we might want to call each callback in a separate event loop tick. That way if one of the callbacks throws the other callbacks will still be called. We could call each callback in nextTick() or setImmediate() or something. (Honestly I don't know too much the difference between them.)

@jomo
Copy link

jomo commented Sep 25, 2015

You might want to add a warning comment where the key variable is assigned / used.

If you change the pattern slightly, e.g to callbackList[type][domain] it will fail when the domain matches an Object.prototype property/method name, such as 'constructor'.

We used the same pattern for concurrent requests in a different project and only noticed when it crashed.

@drewfish
Copy link
Contributor

drewfish commented Aug 1, 2017

You might also consider using the inflight module:
https://www.npmjs.com/package/inflight

@goloroden
Copy link

Any update on this?

Is there a special reason why this PR was not merged? Is there anything one can do to help?

@drewfish
Copy link
Contributor

drewfish commented Feb 2, 2018

@goloroden This PR has received a bunch of comments but hasn't been updated, so it appears to be abandoned. (As well there's a merge conflict which needs to be addressed.) If you (or anyone else) wish to submit a different PR which accomplishes the same thing we'd be happy to work to get it merged.

@goloroden
Copy link

Okay, thanks!

@goloroden
Copy link

goloroden commented Sep 28, 2019

@cloudrac3r Would you mind closing this PR now that #30 has landed (it still appears in the list of PRs I should know about, which is not needed 😉)?

@cloudrac3r
Copy link
Contributor

@goloroden Sorry, what do you want me to do? I am the author of 30, I am not the author of this one.

@goloroden
Copy link

Sorry, apparently I mixed things up! Didn’t want to bother you 😔

This should probably rather be gone to @drewfish or @esnunes

@esnunes esnunes closed this Oct 8, 2019
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.

None yet

7 participants