Opened 9 years ago

Closed 9 years ago

#698 closed defect (fixed)

ModuleCCSession object may group_unsubscribe in the closed CC session in being deleted

Reported by: naokikambe Owned by: naokikambe
Priority: medium Milestone: Sprint-20110405
Component: message-library Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description


Subtickets

Change History (17)

comment:1 Changed 9 years ago by naokikambe

  • Summary changed from ModuleCCSession may operate the closed CC session in being deleted to ModuleCCSession object may group_unsubscribe in the closed CC session in being deleted

In trac547, we can see this verbose massage in boss shutting down.

Exception isc.cc.session.SessionError: SessionError('Session has been closed.',) in <bound method ModuleCCSession.__del__ of <isc.config.ccsession.ModuleCCSession object at 0x9f926ac>> ignored

But note that this is an independent defect from trac547. This is because that the ModuleCCSession object does group_unsubscribe in the closed CC session when this object is being deleted. To avoid this, we need to add the lines in __del__ function that detects whether CC session has been closed and returns immediately if it's closed. This is a proposed patch.

--- a/src/lib/python/isc/config/ccsession.py
+++ b/src/lib/python/isc/config/ccsession.py
@@ -152,6 +152,9 @@ class ModuleCCSession(ConfigData):
         self._remote_module_configs = {}
 
     def __del__(self):
+        # If the CC Session obejct has been closed, it returns
+        # immediately.
+        if self._session._closed: return
         self._session.group_unsubscribe(self._module_name, "*")
         for module_name in self._remote_module_configs:
             self._session.group_unsubscribe(module_name)

comment:2 Changed 9 years ago by naokikambe

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

I committed the proposed patch as branch trac698 including a proposed ChangeLog? entry. It turns to reviewing.

comment:3 Changed 9 years ago by stephen

  • Milestone feature backlog item deleted

Milestone feature backlog item deleted

comment:4 Changed 9 years ago by shane

  • Milestone set to Sprint-20110405

comment:5 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to naokikambe

Hello

For one, the variable _closed is not needed. If you look into the close method:

         # need to pass along somehow that this function has been called,
         self._socket = "closed"

So it seems that there's another variable carrying the same information.

And there's another problem. There are no tests for the code.

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

  • Owner changed from naokikambe to vorner

Thanks for comments.

Replying to vorner:

So it seems that there's another variable carrying the same information.

I think it is not definitely same information. According to the original class (isc.cc.session.Session), "_socket" variable is a socket object, "_closed" variable is boolen, which means whether the session has been closed or not.

However, in unittest_fakesession.py, "_closed" variable is a string type(not a socket object), and it is assigned to "closed" string which seems to be same meaning as "_closed" variable here. So we will assign None instead of "closed" string. Is that OK?

         # need to pass along somehow that this function has been called,
-        self._socket = "closed"
+        self._socket = None

And there's another problem. There are no tests for the code.

unittest_fakesession.py is a test code. I think we don't need test code for test code. Is that correct?

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

Replying to naokikambe:

And there's another problem. There are no tests for the code.

unittest_fakesession.py is a test code. I think we don't need test code for test code. Is that correct?

Sorry, I had misunderstanding. I think unittest_fakesession.py is a STUB code for the session class. Don't we need test for the stub code?

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

  • Owner changed from vorner to naokikambe

Hello

The None is OK. I kind of thought that it's the same file (I was looking at the diff). But it isn't that much important.

The fakesession is stub code, but the ccsession.py isn't, and that one should have a test (because if the test was passed before, this change is not tested).

With regards

comment:10 in reply to: ↑ 9 ; follow-up: Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to vorner

Hello,

Replying to vorner:

The fakesession is stub code, but the ccsession.py isn't, and that one should have a test (because if the test was passed before, this change is not tested).

I think this change is already tested at test_close in ccsession_test.py. Is that correct?

    def test_close(self):
        fake_session = FakeModuleCCSession()
        mccs = self.create_session("spec1.spec", None, None, fake_session)
        mccs.close()
        self.assertEqual("closed", fake_session._socket)

Further more, we may surely know this change is applied effectively if we add the following to unittest_fakesession.py. Is that okay?

@@ -44,6 +44,8 @@ class FakeModuleCCSession:
             self.subscriptions[group_name].append(instance_name)

     def group_unsubscribe(self, group_name, instance_name = None):
+        if self._closed:
+            raise isc.cc.SessionError("Session has been closed.")
         if group_name in self.subscriptions:
             if instance_name:
                 if len(self.subscriptions[group_name]) > 1:

However I think the unittest never fails without this change even if this exception is raised. Because this exception in deleting the object is just ignored like this. Is that correct?

for pytest in ccsession_test.py cfgmgr_test.py config_data_test.py module_spec_test.py ; do \
echo Running test: $pytest ; \
env PYTHONPATH=/home/kambe/git/src/lib/python:/home/kambe/git/src/lib/python \
CONFIG_TESTDATA_PATH=/home/kambe/git/src/lib/config/tests/testdata \
CONFIG_WR_TESTDATA_PATH=/home/kambe/git/src/lib/config/tests/testdata \
python3-coverage run --branch --append /home/kambe/git/src/lib/python/isc/config/tests/$pytest || exit ; \
done
Running test: ccsession_test.py
...................[['ConfigManager', None, {'command': ['get_config', {'module_name': 'Spec2'}]}]]
..Exception isc.cc.session.SessionError: SessionError('Session has been closed.',) in <bound method ModuleCCSession.__del__ of <isc.config.ccsession.ModuleCCSession object at 0x9199c2c>> ignored
..........[Spec2] Error requesting configuration: just an error
....
----------------------------------------------------------------------
Ran 35 tests in 0.038s

OK

Thanks,

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

Good morning

Replying to naokikambe:

Further more, we may surely know this change is applied effectively if we add the following to unittest_fakesession.py. Is that okay?

@@ -44,6 +44,8 @@ class FakeModuleCCSession:
             self.subscriptions[group_name].append(instance_name)

     def group_unsubscribe(self, group_name, instance_name = None):
+        if self._closed:
+            raise isc.cc.SessionError("Session has been closed.")
         if group_name in self.subscriptions:
             if instance_name:
                 if len(self.subscriptions[group_name]) > 1:

Yes, that woold be good.

However I think the unittest never fails without this change even if this exception is raised. Because this exception in deleting the object is just ignored like this. Is that correct?

If we created a test where the object was created, closed and force-deleted, the exception should be raised in the case it is not properly checked (eg. without the fix). Something like this (not exact code) could do the trick, but I didn't try it:

# Get the session somewhere
session.close()
session = None
gt.collect() # Force garbage collection, which forces deletion of the object

Would that be possible?

Thanks

comment:12 Changed 9 years ago by vorner

  • Owner changed from vorner to naokikambe

comment:13 in reply to: ↑ 11 Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to vorner

Hello,

Replying to vorner:

However I think the unittest never fails without this change even if this exception is raised. Because this exception in deleting the object is just ignored like this. Is that correct?

If we created a test where the object was created, closed and force-deleted, the exception should be raised in the case it is not properly checked (eg. without the fix). Something like this (not exact code) could do the trick, but I didn't try it:

# Get the session somewhere
session.close()
session = None
gt.collect() # Force garbage collection, which forces deletion of the object

Would that be possible?

No. I could not make it failed. (Result did not change.) However I could make it failed by adding another test case like following.

    def test_del(self):
        fake_session = FakeModuleCCSession()
        mccs = self.create_session("spec1.spec", None, None, fake_session)
        mccs.close()
        mccs.__del__()

Is that OK ?

Thanks,

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

  • Owner changed from vorner to naokikambe

Yes, it would be OK. I just guessed a way to make it deleted, yours seem shorter and more straight-forward.

Thanks

comment:15 in reply to: ↑ 14 Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to vorner

I pushed at git e1e592789ebc8806b9a8767af95c41c1ec2d5a14. Please check the changes again.
thanks,

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

  • Owner changed from vorner to naokikambe

Hello

I didn't really like the fact that single object was deleted twice, it looked wrong. So I split the test into two and pushed.

If you agree with the change, you can merge it.

Thanks

comment:17 in reply to: ↑ 16 Changed 9 years ago by naokikambe

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

Thank you!
I had merged it and I'm closing the ticket.

Note: See TracTickets for help on using tickets.