Opened 8 years ago

Closed 8 years ago

#1542 closed defect (fixed)

socket creator protocol should distinguish socket level and other errors

Reported by: jinmei Owned by: vorner
Priority: very high Milestone: Sprint-20120207
Component: ~Boss of BIND (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: Socket creator
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 3.5 Internal?: no

Description

Currently (based on master and #1522), if (e.g.) a requested socket
cannot be created or bound to the specified address/port (such as
due to missing privilege or duplicate use), the boss simply returns a
CC message with the rcode of 1. The requestor code will then throw
CCSessionError.

Since the non 0 return code and CCSessionError from the requestor can
happen for other unusual errors (such as connection reset or bogus
return data from the boss, possibly due to a bug), the application
won't be able to detect how serious this situation is and how/whether
to recover from this situation.

The boss should return a specific result code or something for the
possible and more likely-to-happen errors like a socket binding
failure from other errors (e.g., it could return a specific non 0
rcode for this purpose). The requestor should also distinguish these
cases for the return value to the caller.

I'd personally suggest using a non exception way for socket level
errors, but it could also be a separate exception than CCSessionError
and SocketError?.

Subtickets

Change History (12)

comment:1 Changed 8 years ago by jinmei

  • Feature Depending on Ticket set to Socket creator

comment:2 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120207

comment:4 Changed 8 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:5 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

I used exceptions (because it is easier to code, can contain more information and the code using it doesn't need to be changed (yet) to reflect the change). I derived a NonFatalSocketErorr? (and used it as a base for two other exceptions, so the user of the function can have more details if he wants so) from the SocketError?.

Should the code using it do something special now? Because currently it just rejects the configuration, no matter which error it is.

I'd propose this as a changelog entry:

[func]		vorner
The SocketRequestor provides more information about what error happened when
it throws, by using subclasses of the original exception. This way a user
not interested in the difference can still use the original exception, while
it can be recognized if necessary.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:7 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by jinmei

Replying to vorner:

First off, I've fixed one trivial error in the branch.

I used exceptions (because it is easier to code, can contain more information and the code using it doesn't need to be changed (yet) to reflect the change). I derived a NonFatalSocketErorr? (and used it as a base for two other exceptions, so the user of the function can have more details if he wants so) from the SocketError?.

I personally don't like to overuse exceptions for something we can
expect (and in either case it's better if we have some consistent
guideline of when to use exceptions in this context (i.e. not in the
safety vs resilience context)), but that's probably a separate general
discussion. So I won't fight against it in the context of this
ticket.

Should the code using it do something special now? Because currently it just rejects the configuration, no matter which error it is.

The user part should probably be left to a separate ticket, because
it's related to a topic of how to revise port binding configuration.
But I'd like to make sure that one is also handled soon rather than
just using it as an easy excuse of not doing it here.

I think there's already a related ticket. If not we need to create
one.

As for the branch itself, I have just a few minor comments:

general

I'd like to avoid hardcode numbers like "2" or "3", and ideally
centralize the definition of these. one possible way is to introduce a
central C++ header file and share the definition for python via a
binding, but in this case this might be overkilling. So defining them
both in C++ and in python consistently may be a compromise.

bind10_test

Shouldn't we also update check_code() so that it will check other
cases of non 0 code?

socket_request

  • I'd like to explicitly clarify that NonFatalSocketError? doesn't break the socket state (so the caller can safely keep using it).
  • s/operation system/operating system/?
        /// \brief Exception when the operation system doesn't allow us to create
        ///    the requested socket.
    

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:9 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I used exceptions (because it is easier to code, can contain more information and the code using it doesn't need to be changed (yet) to reflect the change). I derived a NonFatalSocketErorr? (and used it as a base for two other exceptions, so the user of the function can have more details if he wants so) from the SocketError?.

I personally don't like to overuse exceptions for something we can
expect (and in either case it's better if we have some consistent
guideline of when to use exceptions in this context (i.e. not in the
safety vs resilience context)), but that's probably a separate general
discussion. So I won't fight against it in the context of this
ticket.

Well, I understand exceptions like a tool for handling unusual code flows, when we need to abort some operation, possibly on multiple levels of the stack. This is exactly the case, no matter if we can expect such an abort or not (in theory, we should expect an error/exception to happen anywhere).

But yes, we might want to talk about it outside of the ticket, if we need some kind of consensus.

general

I'd like to avoid hardcode numbers like "2" or "3", and ideally
centralize the definition of these. one possible way is to introduce a
central C++ header file and share the definition for python via a
binding, but in this case this might be overkilling. So defining them
both in C++ and in python consistently may be a compromise.

Hmm, right. I declared them for the code, but left the magic numbers in for the tests, so it would be caught if the constants were set to wrong numbers.

Thank you

comment:10 in reply to: ↑ 9 Changed 8 years ago by jinmei

Revised code looks okay. Please feel free to merge.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner
  • Total Hours changed from 0 to 0.75

comment:12 Changed 8 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0.75 to 3.5

Thanks, merged, closing.

Note: See TracTickets for help on using tickets.