[nanomsg] Re: [PATCH] Terminate astream if initializing usock failed
- From: Nir Soffer <nirsof@xxxxxxxxx>
- To: nanomsg@xxxxxxxxxxxxx
- Date: Tue, 12 Mar 2013 15:56:51 +0200
On Tue, Mar 12, 2013 at 3:38 PM, Martin Sustrik <sustrik@xxxxxxxxxx> wrote:
> On 2013-03-12 14:28, Nir Soffer wrote:
>
>> On Tue, Mar 12, 2013 at 1:09 PM, Martin Sustrik <sustrik@xxxxxxxxxx>
>> wrote:
>>
>> On 2013-03-12 11:49, Nir Soffer wrote:
>>>
>>> On Tue, Mar 12, 2013 at 12:09 PM, Martin Sustrik <sustrik@xxxxxxxxxx>
>>>> wrote:
>>>>
>>>> On 2013-03-12 10:59, Nir Soffer wrote:
>>>>
>>>>>
>>>>> On OS X the scenario is this:
>>>>>
>>>>>
>>>>>> 1. client connects and close the socket
>>>>>> 2. server accept a new socket
>>>>>> 3. server tune the socket
>>>>>> 4. setsockopt fail with EINVAL (looks like a bug in OS X, as the value
>>>>>> is
>>>>>> valid (e.g. 128K receive buffer)
>>>>>>
>>>>>>
>>>>>> Yuck. That's an ugly behaviour. Anyway, what if we simply ignored the
>>>>> EINVAL error from all those setsockopts, when on OSX?
>>>>>
>>>>>
>>>>
>>>> What will happen if we fail to make the socket non-blocking, and then we
>>>> try to initialize the stream with a blocking socket?
>>>>
>>>> I think that failing fast is better in most cases. What I don't like
>>>> about
>>>> this patch is that we cannot detect a bug in the library. For example,
>>>> if
>>>> we change the code to set a (real) invalid value, we will simply fail to
>>>> handle all connections, instead of asserting.
>>>>
>>>>
>>> Yeah. I hate that we can't distinguish real "errorneous value" problem
>>> (nanomsg should assert) from "peer disconnected" problem (nanomsg should
>>> close the socket in decent manner).
>>>
>>> Anyway, given that we are not able to distinguish between the two, I
>>> think
>>> that only option is to assume that there's no bug in the related library
>>> code and consider the error to mean "peer disconnected".
>>>
>>>
>>> What if we check EINVAL, then fail with -EINVAL on OS X (and other
>>>
>>>> platform
>>>> that show this error), and assert on Linux?
>>>>
>>>>
>>> AFAICS it's the same as ignoring the error on OSX.
>>>
>>
>>
>> But on other platforms - assuming that it never happens there, we can now
>> detect a bug in the library.
>>
>
> Yes. But why not do it this way:
>
> rc = setsockopt (...);
> #ifdef NN_HAVE_OSX
> /* Due to bug on OSX, EINVAL may mean that the peer has disconnected.
> Ignore it and assume send/recv functions will report the error later
> on. */
> errno_assert (rc == 0 || errno == EINVAL);
> #else
> errno_assert (rc == 0);
> #endif
>
> That way we don't have to change prototype of nn_uscok_tune() function
> just because there's a bug in OSX.
>
> Generally speaking, I would like to have code dealing with platform bugs
> and deficiencies to have as little scope as possible.
We can do this - but you have to repeat this for all setsockopt calls and
for fnctl call. And It may cause trouble later in nn_stream_init if the
socket is blocking.
Changing nn_sock_tune to return an error is good anway - it is eaiser to
change error handling later if needed.
Other related posts:
