Robust ServerSocket not throw (EOL) on recoverable accept failure #10386

sgothel · 2024-10-31T14:11:54Z

Resolves: ...
Target version: master

Summary

ServerSocket shall not throw (EOL) on accept returning nullptr
Simplify accept, ending application on unrecoverable error

Have ServerSocket::accept determine whether to Util::forcedExit(EX_SOFTWARE)
on unrecoverable errors:

Exit application if ::accept returns an unrecoverable errno (see below)
- tolerate Socket ctor exceptions
- tolerate ::accept recoverable errors (see below)
Note, the following are no throw
- ::close
- ::inet_ntop

Also reject exceeding external connections right within ServerSocket::accept,
avoiding creation of Socket objects - hence saving resources.

Recoverable ::accept errno values
following accept(2) are

EAGAIN // == EWOULDBLOCK
ENETDOWN // man accept(2): to be treated like EAGAIN
EPROTO // man accept(2): to be treated like EAGAIN
ENOPROTOOPT // man accept(2): to be treated like EAGAIN
EHOSTDOWN // man accept(2): to be treated like EAGAIN
ENONET // man accept(2): to be treated like EAGAIN (undef on BSD, Apple, ..)
EHOSTUNREACH // man accept(2): to be treated like EAGAIN
EOPNOTSUPP // man accept(2): to be treated like EAGAIN
ENETUNREACH // man accept(2): to be treated like EAGAIN
ECONNABORTED // OK
ETIMEDOUT // OK, should not happen due ppoll

May consider handling temporary EMFILE, ENFILE, ENOBUFS and ENOMEM errors?

Unit Testing

Unit testing hooks for failure injection is provided via

UnitWSD::simulateExternalAcceptError()
UnitWSD::simulateExternalSocketCtorException()

and utilized in

test/UnitServerSocketAcceptFailure1.cpp
test/UnitStreamSocketCtorFailure1.cpp

Checklist

I have run make prettier-write and formatted the code.
All commits have Change-Id
I have run tests with make check
I have issued make run and manually verified that everything looks okay
Documentation (manuals or wiki) has been updated or is not required

mmeeks

the connection counting code is now looking good.
Getting it in is blocked by a) CI - and b) now fixing up the ServerSocket change here to make it much smaller and more helpful I think.
Thanks.

net/ServerSocket.hpp

net/Socket.cpp

sgothel · 2024-10-31T18:25:37Z

This is the ServerSocket fix vanilla branch w/o merged max-connection
https://github.com/CollaboraOnline/online/tree/private/sgothel/sockrepair4_p1

This is the ServerSocket fix branch w/ merged max-connection (this reviewed one)
https://github.com/CollaboraOnline/online/tree/private/sgothel/sockrepair4_p2

I applied all review remarks .. but not fully removed the 'swizzle around' lines
for clarity of the resulting code, please see
https://github.com/CollaboraOnline/online/blob/private/sgothel/sockrepair4_p2/net/Socket.cpp#L1153

mmeeks

I'd like to see unit tests for this - possibly we need a fault injection hook in UnitBase to throw during SSL socket creation or something - and of course for when we run out of sockets too.
Thanks!

sgothel · 2024-11-06T15:03:58Z

I'd like to see unit tests for this - possibly we need a fault injection hook in UnitBase to throw during SSL socket creation or something - and of course for when we run out of sockets too. Thanks!

Revised pull request including unit tests injection

recoverable ::accept errors
non-recoverable ::accept errors
recoverable (server) StreamSocket ctor exceptions

Classification of recoverable ::accept errors has been revised and documented.

sgothel · 2024-11-06T15:04:54Z

Unit test logs w/ error injections

caolanm · 2024-11-06T16:25:15Z

ENONET doesn't appear to exist on IOS, presumably an easy fix for that build error

sgothel · 2024-11-06T18:25:44Z

ENONET doesn't appear to exist on IOS, presumably an easy fix for that build error

thx, checked on FreeBSD's accept(2) as well as on their errno.h + Apple's. So it was just this one missing on BSD + Apple.

mmeeks

Needs some more work; thanks.

net/Socket.cpp

net/Socket.hpp

net/Socket.cpp

commit 94f2825 removed WebSocketHandler's WS Ping timeout functionality and disabled the unit test. Signed-off-by: Sven Göthel <[email protected]> Change-Id: I6c8a805437f66346ea3feac421b3a15707e7417f

mmeeks

Nearly there.

net/Socket.cpp

net/ServerSocket.hpp

net/Socket.cpp

mmeeks

Sorry missed a couple more here; hopefully next round we'll get it in!

net/Socket.cpp

mmeeks · 2024-11-12T17:59:18Z

Otherwise - much better of course.

…ify accept. Have `ServerSocket::accept` determine whether to `Util::forcedExit(EX_SOFTWARE)` on unrecoverable errors: - Exit application if `::accept` returns an unrecoverable `errno` (see below) - tolerate `Socket` ctor exceptions - tolerate `::accept` recoverable errors (see below) - Note, the following are `no throw` - `::close` - `::inet_ntop` Also reject exceeding external connections right within `ServerSocket::accept`, avoiding creation of `Socket` objects - hence saving resources. Recoverable `::accept` `errno` values following [accept(2)](https://www.man7.org/linux/man-pages/man2/accept.2.html) are - EAGAIN // == EWOULDBLOCK - ENETDOWN // man accept(2): to be treated like EAGAIN - EPROTO // man accept(2): to be treated like EAGAIN - ENOPROTOOPT // man accept(2): to be treated like EAGAIN - EHOSTDOWN // man accept(2): to be treated like EAGAIN - ENONET // man accept(2): to be treated like EAGAIN (undef on BSD, Apple, ..) - EHOSTUNREACH // man accept(2): to be treated like EAGAIN - EOPNOTSUPP // man accept(2): to be treated like EAGAIN - ENETUNREACH // man accept(2): to be treated like EAGAIN - ECONNABORTED // OK - ETIMEDOUT // OK, should not happen due ppoll We also allow retrying on temporary EMFILE, ENFILE, ENOBUFS and ENOMEM errors, which may also be candidates for later fine grained control in case of resource degradation. Unit testing hooks for failure injection is provided via - UnitWSD::simulateExternalAcceptError() - UnitWSD::simulateExternalSocketCtorException() and utilized in - test/UnitServerSocketAcceptFailure1.cpp - test/UnitStreamSocketCtorFailure1.cpp Signed-off-by: Sven Göthel <[email protected]> Change-Id: I939df8e8462d2c29d19214a9b5fb70ad37dc7500

sgothel · 2024-11-13T08:32:08Z

missed one `amend'

mmeeks

Lovely thank you - didn't read the unit tests much; but really glad to see them =)

mmeeks requested changes Oct 31, 2024

View reviewed changes

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from 974b9a9 to 44f01da Compare October 31, 2024 18:22

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from 44f01da to 641a07d Compare November 1, 2024 10:38

sgothel changed the title ~~Implement MaxConnection Limit (2c)~~ Robust ServerSocket not throw (EOL) on recoverable accept failure Nov 1, 2024

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch 2 times, most recently from a8f061b to b8f548b Compare November 1, 2024 17:48

mmeeks requested changes Nov 1, 2024

View reviewed changes

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from b8f548b to 0a29005 Compare November 6, 2024 14:58

sgothel marked this pull request as ready for review November 6, 2024 15:04

sgothel requested a review from mmeeks November 6, 2024 15:05

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from 0a29005 to 98ae285 Compare November 6, 2024 15:25

sgothel requested a review from caolanm November 6, 2024 15:31

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from 98ae285 to a6a1ab5 Compare November 6, 2024 18:11

mmeeks requested changes Nov 7, 2024

View reviewed changes

net/Socket.cpp Outdated Show resolved Hide resolved

net/Socket.cpp Outdated Show resolved Hide resolved

net/Socket.cpp Outdated Show resolved Hide resolved

net/Socket.cpp Outdated Show resolved Hide resolved

net/Socket.hpp Outdated Show resolved Hide resolved

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch 3 times, most recently from 36dad03 to 30a8c40 Compare November 8, 2024 13:24

caolanm reviewed Nov 8, 2024

View reviewed changes

net/Socket.cpp Outdated Show resolved Hide resolved

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from 30a8c40 to 7fcb554 Compare November 8, 2024 16:19

sgothel requested a review from mmeeks November 11, 2024 15:16

Remove test/UnitTimeoutWSPing.cpp

558a4fd

commit 94f2825 removed WebSocketHandler's WS Ping timeout functionality and disabled the unit test. Signed-off-by: Sven Göthel <[email protected]> Change-Id: I6c8a805437f66346ea3feac421b3a15707e7417f

caolanm force-pushed the private/sgothel/sockrepair4_p2 branch from 7fcb554 to 1b5ebe3 Compare November 11, 2024 19:27

mmeeks requested changes Nov 12, 2024

View reviewed changes

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from 1b5ebe3 to 99142ee Compare November 12, 2024 14:22

sgothel requested a review from mmeeks November 12, 2024 14:22

mmeeks requested changes Nov 12, 2024

View reviewed changes

net/Socket.cpp Outdated Show resolved Hide resolved

net/Socket.cpp Outdated Show resolved Hide resolved

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from 99142ee to 82409cc Compare November 13, 2024 08:29

sgothel force-pushed the private/sgothel/sockrepair4_p2 branch from 82409cc to 4dbee2e Compare November 13, 2024 08:31

sgothel requested a review from mmeeks November 13, 2024 09:50

mmeeks approved these changes Nov 13, 2024

View reviewed changes

mmeeks merged commit 2fbdc2a into master Nov 13, 2024
13 checks passed

mmeeks deleted the private/sgothel/sockrepair4_p2 branch November 13, 2024 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Robust ServerSocket not throw (EOL) on recoverable accept failure #10386

Robust ServerSocket not throw (EOL) on recoverable accept failure #10386

sgothel commented Oct 31, 2024 •

edited

Loading

mmeeks left a comment

sgothel commented Oct 31, 2024

mmeeks left a comment

sgothel commented Nov 6, 2024

sgothel commented Nov 6, 2024

caolanm commented Nov 6, 2024

sgothel commented Nov 6, 2024

mmeeks left a comment

mmeeks left a comment

mmeeks left a comment

mmeeks commented Nov 12, 2024

sgothel commented Nov 13, 2024

mmeeks left a comment

Robust ServerSocket not throw (EOL) on recoverable accept failure #10386

Robust ServerSocket not throw (EOL) on recoverable accept failure #10386

Conversation

sgothel commented Oct 31, 2024 • edited Loading

Summary

Unit Testing

Checklist

mmeeks left a comment

Choose a reason for hiding this comment

sgothel commented Oct 31, 2024

mmeeks left a comment

Choose a reason for hiding this comment

sgothel commented Nov 6, 2024

sgothel commented Nov 6, 2024

caolanm commented Nov 6, 2024

sgothel commented Nov 6, 2024

mmeeks left a comment

Choose a reason for hiding this comment

mmeeks left a comment

Choose a reason for hiding this comment

mmeeks left a comment

Choose a reason for hiding this comment

mmeeks commented Nov 12, 2024

sgothel commented Nov 13, 2024

mmeeks left a comment

Choose a reason for hiding this comment

sgothel commented Oct 31, 2024 •

edited

Loading