search

amphp / process

An async process dispatcher for Amp.
MIT License
235 stars 30 forks source link

Defer handshake timeout in case the event loop is completely blocked #24

Closed kelunik closed 6 years ago

kelunik commented 6 years ago

Fixes #21.

@Bilge please try with this branch.

Bilge commented 6 years ago

Unfortunately this appears to have had no effect. Possible concurrency before failing due to timeout is exactly the same as before this patch.

kelunik commented 6 years ago

@Bilge Does the new patch change anything?

Bilge commented 6 years ago

Yes! :ok_hand: I can now achieve concurrency up to about ~1500 without changing the timeout. Anything more than that and I just run out of memory, but timeouts no longer seem to occur.

Bilge commented 6 years ago

Aside, I'm not sure force pushing is really such a good idea. Obviously your older solution was not the answer but now there's no record it ever existed. It's usually a good idea to keep old ideas around if only for posterity.

kelunik commented 6 years ago

@Bilge Nobody needs a non-working fix for an issue. ;-)

It's basically the same idea, but applied a bit differently.

Bilge commented 6 years ago

One day you might want to refer back to something you tried that didn't work. Just a thought; I don't mean to tell you how to run your projects.

kelunik commented 6 years ago

@Bilge There are cases where keeping the history is beneficial, but not in this case. The new implementation is both more elegant and actually correct. ;-)

Bilge commented 6 years ago

@kelunik Is it not possible to write a test case for this? It would be unfortunate if this were to creep back in as a regression later.

kelunik commented 6 years ago

@Bilge Definitely! Do you want to provide a PR? Basically it just needs to start a process and then directly call sleep(2); before yielding $process->join();.

Bilge commented 6 years ago

That doesn't sound as difficult as I was imagining, but I'm still not sure it is within my ability.

kelunik commented 6 years ago

@Bilge Just push a PR with what you have and we can improve it together. :-)