Opened 8 years ago

Closed 8 years ago

#275 closed enhancement (fixed)

refactoring CC session class for easier tests

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: 06. 4th Incremental Release
Component: ~Inter-module communication(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?: no

Description

I propose a refactoring based on r2393 (part of trac #221).

The main idea is:

  • introduce a base abstract class of isc::cc::Session
  • other modules that use cc Session can develop tests more easily by using a derived mock Session class
  • it will be also useful if/when we want to add support for a "stand alone" mode in a particular process. I can think of this feature for the auth server.
  • this is also a good opportunity to clean up non-asio mode of the Session class. it will make the code much simpler
  • further, I'd make the config module independent from ASIO as part of this refacotring

Subtickets

Change History (8)

comment:1 Changed 8 years ago by jinmei

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

branches/trac275 is ready for review.

The diff is pretty big, but I believe (hope) it's quite straightforward.

I forgot to mention one more good sideeffect of this change: we now don't need the "getSocket()" method in ModuleCCSession and (Abstract)Session.

Proposed ChangeLog entry is as follows:

  72	[func]*		jinmei
	Refactored the cc::Session class by introducing an abstract base
	class.  Test code can use their own derived mock class so that
	tests can be done without establishing a real CC session.  This
	change also modified some public APIs, mainly in the config
	module. (Trac #275, rTBD)

I think Jelte would be the best reviewer for this change, so I'm tentatively assigning it to him.

comment:2 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

The header file documentation was apparently already lagging behind the actual api, but now even more so :) (especially some information in ccsession.h about how the session should be passed, and what a caller would have to do, or that the caller would not be expected to do anything, which seems to be the case, since establish() is called in the constructor of CCSession).

For that matter, should we think about having another one where the user doesn't have to pass a Session, and let the ModuleCCSession take care of it, in case the caller really doesn't care?

For the sake of consistency, while not exactly essential, should we make *ccsession and *cs stack objects in auth's main.cc (and perhaps then rethink how we can do the same for auth_srv and io_service)?

auth_srv.h declares a setSession() now, but it doesn't actually seem to be defined anywhere (or used, obviously :)) I suspect that this is a leftover and should be removed.

Rest looks good.

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

Replying to jelte:

The header file documentation was apparently already lagging behind the actual api, but now even more so :) (especially some information in ccsession.h about how the session should be passed, and what a caller would have to do, or that the caller would not be expected to do anything, which seems to be the case, since establish() is called in the constructor of CCSession).

I've updated the description of the constructor. But now that I've read the comment, I wonder it would be more natural if the caller must establish the session. As a related matter, maybe we should explicitly detect and reject an attempt of doubly establishing a session? (I guess the underlying ASIO implementation detects it, but we should probably catch the invalid usage at a higher level).

For that matter, should we think about having another one where the user doesn't have to pass a Session, and let the ModuleCCSession take care of it, in case the caller really doesn't care?

I'm not so sure about that. To allow this, we should still pass an io_service (or our own wrapper object to ASIO) to the ModuleCCSession constructor or have ModuleCCSession construct an io_service internally. The former wouldn't be so convenient compared to the current constructor; the latter wouldn't be practical very much because in many cases objects like io_service would have to be managed in the top level application.

In any case, we don't need this variation right now, so I'd suggest leaving the decision open for now.

For the sake of consistency, while not exactly essential, should we make *ccsession and *cs stack objects in auth's main.cc (and perhaps then rethink how we can do the same for auth_srv and io_service)?

I'm not sure what this means. Do you mean defining cc_session and cs in the anonymous namespace of auth/main.cc where auth_server and io_service are defined? If so, I'd actually rather define the latter set more locally (because in general it's better to use a narrower scope for mutable variables). But for now we need to define them in the namespace scope because they are referenced from free functions (my_config_handler and my_command_handler). As a middle term plan we'll refactor this architecture with generalizing the ASIO link module, and then I think we can solve this in a cleaner way. So I'd live with the inconsistency for now.

auth_srv.h declares a setSession() now, but it doesn't actually seem to be defined anywhere (or used, obviously :)) I suspect that this is a leftover and should be removed.

Ah, good catch. Actually, it's not a leftover in its usual sense. It will be used in Trac #221, but not necessary in this branch. I removed it.

Is there anything I need to address further, or is it okay to merge it trunk?

comment:4 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:5 Changed 8 years ago by jinmei

Oh, I forgot to mention this.

I made one minor cleanup in session.h (r2444).

Doxygen description update is r2443 and r2445. In r2443 I also changed the type of spec_file_name arg to the ModuleCCSession constructor as a cleanup.

comment:6 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

I've updated the description of the constructor. But now that I've read the comment, I wonder it would be more natural if the caller must establish the session. As a related matter, maybe we should explicitly detect and reject an attempt of doubly establishing a session? (I guess the underlying ASIO implementation detects it, but we should probably catch the invalid usage at a higher level).

Hmm, i think we either need to detect that establish is called twice, or we need to detect that it hasn't been called at all (and then we can choose to error or establish ourselves)

For the sake of consistency, while not exactly essential, should we make *ccsession and *cs stack objects in auth's main.cc (and perhaps then rethink how we can do the same for auth_srv and io_service)?

I'm not sure what this means. Do you mean defining cc_session and cs in the anonymous namespace of auth/main.cc where auth_server and io_service are defined? If so, I'd actually rather define the latter set more locally (because in general it's better to use a narrower scope for mutable variables). But for now we need to define them in the namespace scope because they are referenced from free functions (my_config_handler and my_command_handler). As a middle term plan we'll refactor this architecture with generalizing the ASIO link module, and then I think we can solve this in a cleaner way. So I'd live with the inconsistency for now.

no, i mean not putting them on the heap

    Session* cc_session = NULL;
    <snip>
        cc_session = new Session(io_service->get_io_service());
    <snip>
    delete cc_session;

but on the stack

    Session cc_session(io_service->get_io_service());

And same for ModuleCCSession. Due to the current arch this isn't possible for auth_srv and io_service.

But since we are in a main() function anyway this isn't really important, and we can probably defer this to a refactor later.

If you want to so the check-session-for-established-already here, please do, for the rest i think this is ready for merge.

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

  • billable set to 0
  • Internal? unset

Replying to jelte:

Replying to jinmei:

I've updated the description of the constructor. But now that I've read the comment, I wonder it would be more natural if the caller must establish the session. As a related matter, maybe we should explicitly detect and reject an attempt of doubly establishing a session? (I guess the underlying ASIO implementation detects it, but we should probably catch the invalid usage at a higher level).

Hmm, i think we either need to detect that establish is called
twice, or we need to detect that it hasn't been called at all (and
then we can choose to error or establish ourselves)

Let's think about it later. I suspect a near future refactoring with
a generalized ASIO link will affect the design on this, so it would
make sense to revisit this issue at that point. For the moment, I'll
just leave a comment for the description of the ModuleCCSession
constructor that the requirement is in flux. I'll also make a
separate ticket on this so that we (or trac) can remember the issue.

no, i mean not putting them on the heap

Ah, okay.

And same for ModuleCCSession. Due to the current arch this isn't possible for auth_srv and io_service.

But since we are in a main() function anyway this isn't really important, and we can probably defer this to a refactor later.

As you noted, with the current architecture we can't easily make this
change, and that's the main reason why I used the new-delete pair.
Also noted in the previous comment of mine, I expect we'll improve
this part of the code in a cleaner way in a next step of refactoring.

So, for now, let's live with the current implementation.

If you want to so the check-session-for-established-already here, please do, for the rest i think this is ready for merge.

As noted above, I'd defer it to a separate task. I'm going to merge
the branch to trunk at this point because another important ticket
(xfr/notify) depends on this ticket.

Thanks for the review, btw:-)

comment:8 Changed 8 years ago by jinmei

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

committed to trunk, closing ticket.

Note: See TracTickets for help on using tickets.