RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes

[David Holmes]

On 25/05/2018 3:34 PM, Ivan Gerasimov wrote:
> Hi David!
> 
> 
> On 5/24/18 9:45 PM, David Holmes wrote:
>>
>> I looked but did not review - it was just an observation :)
>>
> 
> Well, thank you anyway :)
> 
>>>
>>>> It seems pointless to double up these condition checks everywhere 
>>>> just in case there is some platform (do we know of one?) where this 
>>>> may be necessary.
>>>>
>>> That's exactly what man pages suggest: "...a portable application 
>>> should check for both..."
>>
>> Yes but that's the native code that calls the library methods. That 
>> doesn't necessarily mean we have to propagate the ambiguity through 
>> our own native wrappers and/or Java code.
>>
> Ah, I didn't immediately understood you're talking about constants in 
> UnixConstants.java and LinuxWatchService.java.
> This part might probably be skipped (because we know that on Linux the 
> constants have the same values), but I thought it's better it add it for 
> consistency.
> 
> In other parts of the fix we do treat the constants uniformly and 
> propagate some non-ambiguous value to Java, like returning 
> IOS_UNAVAILABLE in most cases.
> 
>>> And yes, there exist such platforms.
>>>
>>>> I also wonder whether a smart compiler might not flag code where the 
>>>> errors do infact have the same value:
>>>>
>>>> if (errno == 11 || errno == 11) ...
>>>>
>>> At least gcc -O completely removes the second redundant test, so no 
>>> observable changes is expected on supported platforms.
>>
>> I'm more worried about a new compiler warning - especially if you 
>> happened to use them in a switch! - resulting in future build failures.
>>
> 
> What compiler do you mean: gcc or javac?

gcc

> If gcc, then we already have the same test for both constants in code 
> with no issues.
> For example, java.base/unix/native/libnet/SocketInputStream.c, in 
> NET_ReadWithTimeout():
>          result = NET_NonBlockingRead(fd, bufP, len);
>          if (result == -1 && ((errno == EAGAIN) || (errno == 
> EWOULDBLOCK))) {
> 
> 
> If javac, then, I was thinking about it too, but I don't have a good a 
> universal solution to propose right now.

javac should treat these symbolically rather than based on actual value, 
so I don't see any issue there. It's the C compiler that sees the raw 
value after preprocessing and so sees "duplicate" clauses.

> If one day someone needs to use these (platform dependent by definition) 
> constants in switch, one will need to invent something to workaround the 
> fact that some constants may have the same values on some platforms.
> With respect to EAGAIN and EWOULDBLOCK, it will be caught early enough 
> because it will fail during the very first build on any currently 
> supported Unix platform.

Ok.

Cheers,
David

> 
> With kind regards,
> Ivan
> 
> 
>> Cheers,
>> David
>>
>>> With kind regards,
>>> Ivan
>>>
>>>> Cheers,
>>>> David
>>>>
>>>> On 25/05/2018 6:57 AM, Ivan Gerasimov wrote:
>>>>> Hello!
>>>>>
>>>>> On Unix systems several system calls (including pread, read, readv, 
>>>>> recvfrom, recvmsg, send, sendfile, sendmsg, sendto) may set errno 
>>>>> to either EAGAIN or EWOULDBLOCK on the same condition.
>>>>>
>>>>> On Linux these two constants are the same, but they are not 
>>>>> required to be the same.
>>>>>
>>>>> For example, here's an extract from the Linux man page of send():
>>>>> EAGAIN or EWOULDBLOCK
>>>>> The  socket  is marked nonblocking and the requested operation 
>>>>> would block.  POSIX.1-2001 allows either error to be returned for 
>>>>> this case, and does not require these constants to have the same 
>>>>> value, so a portable application should check for both possibilities.
>>>>>
>>>>> We should check for both error codes when appropriate.
>>>>>
>>>>> Would you please help review the fix?
>>>>>
>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8203369
>>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8203369/00/webrev/
>>>>>
>>>>> Thanks!
>>>>>
>>>>
>>>
>>
>