Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Km/newnode dns prefetch #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

flyinhome
Copy link

Minimal fixes to accommodate newnode_private with tryfirst and dns_prefetch features.

@flyinhome
Copy link
Author

flyinhome commented Nov 23, 2021

Instead of creating two new functions to access the DNS cache, I made the existing evdns_cache_write() and evdns_cache_lookup() functions non-static. Otherwise these are the same as I suggested earlier.

@ghazel
Copy link

ghazel commented Nov 23, 2021

Nice and simple. Let's split the two bug fixes out as well. I'd like to investigate those and submit them upstream.

@flyinhome
Copy link
Author

separate PRs for the bug fixes? ok.

@flyinhome
Copy link
Author

I created separate branches for each of the bug fixes. Hopefully it's not necessary to remove the bug fixes from the km/newnode_dns_prefetch branch.

If memory serves both bugs were found by testing newnode under a debugger, while trying to track down other stubborn bugs. The debugger made it easy to see that a pointer was NULL or a queue was empty.

@ghazel
Copy link

ghazel commented Nov 23, 2021

Hopefully it's not necessary to remove the bug fixes from the km/newnode_dns_prefetch branch.

I would like to remove them from this pull request, because I think they paper over problems elsewhere. For example:

If memory serves both bugs were found by testing newnode under a debugger, while trying to track down other stubborn bugs. The debugger made it easy to see that a pointer was NULL or a queue was empty.

The larger question there is why the queue was empty -- evcons should be tracked in that queue until they are freed. So it is a problem with our usage, or some bug elsewhere.

@flyinhome
Copy link
Author

I tried to verify that the problems that I saw weren't caused by newnode, but I didn't spend hours doing so.

@ghazel
Copy link

ghazel commented Nov 23, 2021

Those two locations do not show up in our crash reports, so at the very least they're new.

@flyinhome
Copy link
Author

It's possible that the evdns callback bug was triggered by the linux implementation of platform_dns_prefetch() (in https_wget.c) calling evdns_getaddrinfo(). I don't know that newnode has needed to use that function before and I suspect libevent apps in general don't need to use it very often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants