Prevent from crashes when url doesn't exists

amphp / dns

Async DNS resolution for PHP based on Amp.

https://amphp.org/dns

MIT License

157 stars 32 forks source link

Prevent from crashes when url doesn't exists #2

Closed estshy closed 10 years ago

estshy commented 10 years ago

Sometimes passed domain is wrong. It crashes pending lookup because there isn't callback in stack.

DaveRandom commented 10 years ago

This looks great, thanks :-)

Just a couple of points:

Do you have a reproduce code sample for this issue? I would like to understand exactly what is going on here, it's possible it may be indicative of a deeper problem.
I suspect it would be possible to check for simply isset($this->pendingLookups[$id]), there shouldn't be a case where the lookup exists but doesn't have a registered callback - again, I would like to fully understand the issue and ensure this fixes it properly.
Fussy cosmetic issue, feel free to ignore: the opening brace for the if block should be on the same line as the if (cond)

estshy commented 10 years ago

I am coding actually great number of staff in laravel framework, where everyone uses opening brace in new line, so it influence me a lot. Let's back to the error, which I am getting when trying to resolve URL http://www.heartofengland.co.uk.

call_user_func() expects parameter 1 to be a valid callback, no array or string given

DaveRandom commented 10 years ago

OK no problem. I'll investigate this later on - I'll probably just merge this straight in anyway as a quick-fix, but I want to see if I can reproduce it first :-)

DaveRandom commented 10 years ago

Merged by cherry pick: https://github.com/DaveRandom/Addr/commit/add4e38c1c6e4ccc95bbb9eda700db5e27a947e4

I did it this way so it can go into lower branches, as master (what will be 0.3) will likely contain a few BC breaks, and this will allow for releases of the stable versions including this fix.

I still haven't got to the bottom of why the issue it possible in the first place, but the fix certainly won't cause any harm so I'm happy for it to go in.

This will screw up you history I'm afraid so you will have to do a forced pull/push to get back in sync - please ensure that any future contributions are made from feature/fix branches in your fork and it will avoid this problem :-)

DaveRandom commented 10 years ago

This fix is now included in releases 0.1.3 and 0.2.2.