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

Load Net::DNS::Native before IO::Socket::IP #1308

Closed
wants to merge 1 commit into from

Conversation

Grinnz
Copy link
Contributor

@Grinnz Grinnz commented Dec 31, 2018

Summary

With the current load order Net::DNS::Native is never used in one setup because it fails to load due to IO::Socket::IP defining non thread safe symbols.

Motivation

Loading it first seems to fix the issue.

References

***********************************************************************
Some package defined non thread safe symbols which Net::DNS::Native uses internally
Please make sure you are not placed loading of modules like IO::Socket::IP
before this one and not called functions like getaddrinfo(), gethostbyname(),
inet_aton() before loading of Net::DNS::Native
************************************************************************

@Grinnz
Copy link
Contributor Author

Grinnz commented Dec 31, 2018

I can't think of any reasonable way to test this.

@Grinnz
Copy link
Contributor Author

Grinnz commented Dec 31, 2018

Through further testing both IO::Socket::IP and Mojo::IOLoop::TLS must be loaded after, probably due to Mojo::IOLoop::TLS loading IO::Socket::SSL.

@Grinnz
Copy link
Contributor Author

Grinnz commented Dec 31, 2018

15:30:13 <Grinnz> https://github.com/mojolicious/mojo/issues/716 some context
15:31:18 <Grinnz> i believe it's because this perl was built without pthreads
15:31:29 <Grinnz> which was not default until ...
15:31:52 <Grinnz> 5.22

#716

@kraih
Copy link
Member

kraih commented Dec 31, 2018

Calling for a vote @mojolicious/core.

@marcusramberg
Copy link
Member

I'm not sure if optimizing for older Perls is worth it. Are there any downsides to the change?

@Grinnz
Copy link
Contributor Author

Grinnz commented Jan 1, 2019

The downsides are the introduced inconsistency in code layout, and that it can only be a partial solution, the setup can still break randomly (by break I mean it just won't use NDN) but at least Mojo::IOLoop::Client can load it in some cases.

@Grinnz
Copy link
Contributor Author

Grinnz commented Jan 1, 2019

For the record, I've worked around this in my code by loading Net::DNS::Native before anything from Mojo.

@jhthorsen
Copy link
Member

I think since it will randomly break, then I'm negative to this change. I think a documentation update would be better.

@jberger
Copy link
Member

jberger commented Jan 1, 2019

As I mentioned on IRC, and agreeing with others, I don't think that because it cannot really address the issue I don't think this change is merited. I agree with @jhthorsen that a documentation patch would be better however I really wonder if this is an upstream issue with NDN and IO::Socket::*. 👎

@kraih
Copy link
Member

kraih commented Jan 1, 2019

Thanks all, with at least two negative votes this pull request cannot pass the vote.

@kraih kraih closed this Jan 1, 2019
@leonerd
Copy link
Contributor

leonerd commented Jan 2, 2019

As author of IO::Socket::IP - is there anything for me to do with this, or that I should know about?

@Grinnz
Copy link
Contributor Author

Grinnz commented Jan 2, 2019

Only what is in the referenced issue #716.

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.

None yet

6 participants