Opened 9 years ago

Closed 9 years ago

#446 closed task (fixed)

configuration knob for in memory data source

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: y2 12 month milestone
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

A new A-team sprint task.

Subtickets

Change History (10)

comment:1 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to reviewing

branches/trac446 is ready for review. I've introduced a generic configuration framework for AuthSrv?, even though it's currently only used for the in memory data source configuration. This is because it's pretty clear we'll soon need more configuration knobs for it. I could actually integrate the existing "database_file" config into this framework, but I didn't do so at this moment to mimize the amount of code to be reviewed.

This is the proposed changelog entry, which explains how to play with it on b10-auth:

  TBD.	[func]*		jinmei
	b10-auth: added a configuration interface to support in memory
	data sources.  For example, the following command to bindctl
	will configure a memory data source containing the "example.com"
	zone with the zone file named "example.com.zone":
	> config set Auth/datasources/ [{"type": "memory", "zones": \
	 [{"origin": "example.com", "file": "example.com.zone"}]}]
	By default, the memory data source is disabled; it must be
	configured explicitly.  To disable it again, specify a null list
	for Auth/datasources:
	> config set Auth/datasources/ []
	Notes: it's currently for class IN only.  The zone files are not
	actually loaded into memory yet (which will soon be implemented).
	This is an experimental feature and the syntax may change in
	future versions.
	(Trac #446, svn rTBD)

comment:2 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:3 follow-up: Changed 9 years ago by vorner

Few comments:

  • Why does destroyAuthConfigParser exist? When I saw it, I asked „Huh, does destroying a config parser need special attention? Why shouldn't I just call delete? What would go wrong?“. As I looked inside it, it does just that, so I think this one is only confusing. Anyway, this function not only should never throw, it just can't, C++ specifies that if anyone tries to throw out of a destructor, the application aborts.
  • Isn't it a lot to ask from commit not to throw any exception? Is it realistic to say that none of them will ever allocate memory, for example?

comment:4 in reply to: ↑ 3 Changed 9 years ago by jinmei

Replying to vorner:

Few comments:

Thanks, these are good ones:-)

  • Why does destroyAuthConfigParser exist? When I saw it, I asked „Huh, does destroying a config parser need special attention? Why shouldn't I just call delete? What would go wrong?“. As I looked inside it, it does just that, so I think this one is only confusing.

I knew this would look oakward, but technically we cannot simply let a module delete an object that were newed in a different module, because these two may use incompatible implementations of new/delete. That's the reason why I introduced the dedicated "destroy" function.

In practice, however, I admit this may be overkilling because normal applications are not supposed to create AuthConfigParser? directly as documented (createAuthConfigParser() is provided as a public function mainly for testing purposes).

Another "tecnically correct" solution is to use a smart pointer and have createAuthConfigParser() return a smart pointer version of the new'ed object; the caller can simply forget the deallocation. I didn't choose this path to reduce the dependency on boost (the only realistic choice of smart pointer in this context would be boost::shared_ptr), but, on rethinking it, it may be okay as this interface is supposed to be used only within our own implementations (and the caller would be tests).

I can think of several possible ways to address your concern:

  1. no code change, but add document about the reason for having a separate function
  2. have the caller delete the returned pointer and document it with the pitfall of new/delete mismatch; remove destroyAuthConfigParser.
  3. use a shared pointer and remove destroyAuthConfigParser.

Does any of the options address the concern? If so, what do you think is the best? If not, do you have a suggestion?

BTW:

Anyway, this function not only should never throw, it just can't, C++ specifies that if anyone tries to throw out of a destructor, the application aborts.

I guess you are talking about this part of doc:

/// This function is not expected to throw an exception unless the underlying
/// implementation of \c parser (an object of a specific derived class of
/// \c AuthConfigParser) throws.

and you mean that, since the only thing destroyAuthConfigParser() does is to do delete (which effectively means calling a destructor in this context), if \c parser throws it should be from a destructor. If so, that's correct. But I'm not sure what you mean by the "C++ specifies ..." part. As far as I know, it's not prohibited by the language spec, although it's widely considered a very bad practice. For example, if you run this code:

struct BadThrower {
    ~BadThrower() { throw 0; } 
};

int main() {
    try {
        BadThrower bt;
    } catch (const int& ex) {
        std::cout << "bad practice, but possible" << std::endl;
    }
    return (0);
}

We'll see this: "bad practice, but possible".

  • Isn't it a lot to ask from commit not to throw any exception? Is it realistic to say that none of them will ever allocate memory, for example?

Yeah, I know it can be a lot. But if we want to implement some way of "safe configuration update", we need some strong requirements like this. By "safe" I mean if an attempt of configuration update fails the old configuration remains valid intact. In other words the commit() variants are a kind of no-throw swap(), which can sometimes be difficult to implement but is crucial in some scenarios.

And, since commit() assumes a strong requirement, I separated it from build(), which can throw and would often need to allocate memory. The idea is to have build() do any work that can fail without touching the server object as much as possible, and then commit() completes the task with much restricted, but safer primitives.

If we think the requirement is too restrictive and unrealistic, possible alternatives would be:

  1. introduce a "cancel()" method (or have the destructor do it), which is responsible for rolling back incomplete configuration committed in the server.
  2. just let commit() throw when it can't avoid that, with a note that the system may become inconsistent state in that case

IMO option 1 would actually be as strong as requireing commit() not throw, so I suspect it's not a realistic option. Option 2 sound irresponsible to me, and I don't like it, but depending on how we handle the configuration stuff in practice, we may be able to live with it as a compromise.

What's your opinion?

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

Do we expect to have incompatible versions of new and delete? I know C++ can overload these, but do we want to do that?

Anyway, as it is used outside the module only inside tests and tests are kind of part of the module itself, I think 2 is the best of them. 1 would be fine too, as the only problem it causes is confusion.

The destructor and exception, OK, I remembered wrong. But I think we have (maybe unwritten) requirement that destructors don't throw, so specifying that they could is somehow wrong. It sounds like we expect them to do so.

With the exception, I think throwing an exception that should not be caught (or, when did, it should be rethrown) is the option. Like if the parser can't avoid it, instead of pretending everything is OK, it would abort the program, but with anything on the path out of main being able to know.

comment:6 in reply to: ↑ 5 Changed 9 years ago by jinmei

Replying to vorner:

Anyway, as it is used outside the module only inside tests and tests are kind of part of the module itself, I think 2 is the best of them. 1 would be fine too, as the only problem it causes is confusion.

Okay, then I'll go with option 2.

As for the new/delete incompatibility:

Do we expect to have incompatible versions of new and delete? I know C++ can overload these, but do we want to do that?

"We" probably don't, "they" may. For public interfaces I generally try to be conservative with fewer assumptions about how it can be used. Also, again technically, even between our own modules the incompatibility could happen if they are built separately with different (versions of) compilers and/or different standard libraries.

The destructor and exception, OK, I remembered wrong. But I think we have (maybe unwritten) requirement that destructors don't throw, so specifying that they could is somehow wrong. It sounds like we expect them to do so.

Point taken. I actually had an undocumented scenario in my mind where this configuration interface is used by external developers to extend the server behavior without modifying the main source code (with some dynamically linkable hooks which are currently not available). Like the case with new/delete, I tend to act conservatively when I expect something to be used by a third party.

Anyway, if our choice is option 2, we'll simply remove these controversial descriptions, so the problem will disappear:-)

With the exception, I think throwing an exception that should not be caught (or, when did, it should be rethrown) is the option. Like if the parser can't avoid it, instead of pretending everything is OK, it would abort the program, but with anything on the path out of main being able to know.

I'm not sure if I understand this suggestion, but on reading this comment my proposal is:

  • still suggest (but not "require") the derived classes of AuthConfigParser? be designed so that commit() is exception free (and build() does any tricky work) in documentation
  • catch all exceptions from AuthConfigParser::commit() in configureAuthServer() and covert them to a FatalError? exception and rethrow it

Does that work for you?

comment:7 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Yes, I agree with that. I had in mind that if they throw, their exception should be derived of the FatalErorr?, but this looks equeally good (and probably is less work for the parser author).

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

  • Owner changed from jinmei to vorner

Replying to vorner:

Yes, I agree with that. I had in mind that if they throw, their exception should be derived of the FatalErorr?, but this looks equeally good (and probably is less work for the parser author).

Okay, I believe I've addressed the comments received so far in r3995 and r3996.

Are these okay, and if so, is there any other concern?

comment:9 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Yes, they look OK. Please, merge.

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

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to vorner:

Yes, they look OK. Please, merge.

Okay, merged. Thanks for review.

Closing ticket.

Note: See TracTickets for help on using tickets.