On 09/05/2012 09:27 AM, Alexander Komyagin wrote: > So you think that it's ok for comm_coonect_addr() to return COMM_OK if > it was called before the appropriate select() notification. Am I right?
Hard to say for sure since comm_coonect_addr() lacks an API description, and there are at least three similar but different ways the function is being used by Squid. One natural way to define this API is to say that it should return COMM_OK if and only if the socket is connected (making any select notifications irrelevant). However, this definition may be too CPU-expensive and/or too unportable to support. And this level of certainty may not actually be needed for current Squid needs! I would not be surprised if there is some gray area where we cannot really tell for sure (without too much additional overhead or portability risk) whether the async socket is connected. The Stevens book seem to imply that much. Inside that gray area, the function should probably return COMM_OK so that the rest of the code works: If we guessed wrong, we will get failures during I/O, but the code has to deal with those anyway. In other words, if you want to work on this, consider defining the API based on current Squid needs and then make sure we support those minimum requirements, keeping overheads and portability in mind. However, I would _start_ by fixing the calling code first (as it may affect the minimum requirements) -- your ConnOpener patch was a step in that direction. HTH, Alex. > On Wed, 2012-09-05 at 08:30 -0600, Alex Rousskov wrote: >> On 09/05/2012 03:32 AM, Alexander Komyagin wrote: >>> On Tue, 2012-09-04 at 09:16 -0600, Alex Rousskov wrote: >> >>>> Again, I hope that this trick is not needed to solve your problem, and I >>>> am worried that it will cause more/different problems elsewhere. I would >>>> recommend fixing CommOpener instead. If _that_ is not sufficient, we can >>>> discuss appropriate low-level enhancements. >>> >>> Both things shall be fixed, IMHO. >> >> If double-connect is not needed, we should not introduce it. AFAICT, >> double-connect is an attempt to cope with a bug in higher-level code. We >> should fix that bug and see if that is sufficient. If it is not >> sufficient, we should evaluate why and only then add appropriate >> low-level code if needed. >> >> The primary goal here is to fix the underlying issue, not just to find a >> workaround (which you have already provided). >> >> >> Thank you, >> >> Alex. >> >
