MonoTime is not being used for credit-time, expires and np_expires #36
Comments
|
Just a quick update. The MonoTime implementation seems incomplete since I've come against a related error in RadiusAccounting.py as well. There might be more but I haven't found them yet. Applying the following two patches seems to fix the issue: diff --git a/sippy/Time/Timeout.py b/sippy/Time/Timeout.py
index 1f0c103..eef98b8 100644
--- a/sippy/Time/Timeout.py
+++ b/sippy/Time/Timeout.py
@@ -41,8 +41,12 @@ def TimeoutInact(timeout_cb, ival, nticks = 1, *cb_params):
return ED2.regTimer(timeout_cb, ival, nticks, False, *cb_params)
def TimeoutAbsMono(timeout_cb, mtime, *cb_params):
+ if isinstance(mtime, int) or isinstance(mtime, float):
+ mtime = MonoTime(monot=mtime)
+
if not isinstance(mtime, MonoTime):
raise TypeError('mtime is not MonoTime')
el = ED2.regTimer(timeout_cb, mtime, None, True, *cb_params)
el.go()
return eldiff --git a/sippy/RadiusAccounting.py b/sippy/RadiusAccounting.py
index 4853e5b..adc0f86 100644
--- a/sippy/RadiusAccounting.py
+++ b/sippy/RadiusAccounting.py
@@ -26,7 +26,7 @@
# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
from time import time, strftime, gmtime
-from sippy.Time.Timeout import Timeout
+from sippy.Time.Timeout import Timeout, MonoTime
sipErrToH323Err = {400:('7f', 'Interworking, unspecified'), 401:('39', 'Bearer capability not authorized'), \
402:('15', 'Call rejected'), 403:('39', 'Bearer capability not authorized'), 404:('1', 'Unallocated number'), \
@@ -167,6 +167,9 @@ class RadiusAccounting(object):
self.global_config['_radius_client'].do_acct(attributes, self._process_result, self.sip_cid, time())
def ftime(self, t):
+ if isinstance(t, MonoTime):
+ return t.ftime()
+
gt = gmtime(t)
day = strftime('%d', gt)
if day[0] == '0':It seems that there is code in some parts that expects floats while at other parts expects MonoTime. However I am not entirely sure especially if the first patch is correct, since I am passing the float value to MonoTime's Could someone help me verify this, and I could submit a PR for both |
|
Hey @twmobius thanks for the follow-up. In general accepting "float" into TimeoutAbsMono() is a wrong direction and a good way to shoot oneself into the foot just masking actual error, since most likely it's just a duration, not really montonic time (i.e. system uptime), so it would just result in the timer far in the past. This check is designed to catch mistakes like this! If you look at the code in UA.startCreditTimer, it explicitly converts from credit_time supplied by the upper layer(s) into MonoTime() by adding it into rtime: Now to actually fix the issue properly I am having a bit of hard time understanding where it happens. Would you mind posting a full traceback leading to hitting this:
And we go from there. Thanks! |
|
Hey @sobomax, thanks for the reply. I was afraid I was alone on this. I totally agree with you, it seemed hackish. The thing is that the issue occurs at I am attaching the radius response Routing commands and their matching exceptions:
The function you've mentioned provided me with the hint I was missing to figure this out. It seems that Line 126 in 8adb5f9 Similarly this is the case with all operation method calls ( And if you look at RadiusAccounting.py b2bua/sippy/RadiusAccounting.py Line 156 in 8adb5f9 the So after changing the operations of MonoTime to return a MonoTime I had to tweak RadiusAccounting here and there to make it work. This is the complete patch: Index: sippy/Time/MonoTime.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sippy/Time/MonoTime.py (revision 8adb5f92a86c7f97122e9613a8ee3022bea83c9e)
+++ sippy/Time/MonoTime.py (date 1603444232432)
@@ -121,24 +121,20 @@
return strftime('%%H:%%M:%%S.000 GMT %%a %%b %s %%Y' % day, gt)
def __add__(self, x):
- if isinstance(x, MonoTime):
- return (self.monot + x.monot)
- return (self.monot + x)
+ offset = x.monot if isinstance(x, MonoTime) else x
+
+ return MonoTime(monot=self.monot + offset)
def __sub__(self, x):
- if isinstance(x, MonoTime):
- return (self.monot - x.monot)
- return (self.monot - x)
+ offset = x.monot if isinstance(x, MonoTime) else x
+
+ return MonoTime(monot=self.monot - offset)
def __radd__(self, x):
- if isinstance(x, MonoTime):
- return (self.monot + x.monot)
- return (self.monot + x)
+ return self.__add__(x)
def __rsub__(self, x):
- if isinstance(x, MonoTime):
- return (x.monot - self.monot)
- return (x - self.monot)
+ return self.__sub__(x)
def __cmp__(self, other):
if other == None:
Index: sippy/RadiusAccounting.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sippy/RadiusAccounting.py (revision 8adb5f92a86c7f97122e9613a8ee3022bea83c9e)
+++ sippy/RadiusAccounting.py (date 1603444232433)
@@ -26,7 +26,7 @@
# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
from time import time, strftime, gmtime
-from sippy.Time.Timeout import Timeout
+from sippy.Time.Timeout import Timeout, MonoTime
sipErrToH323Err = {400:('7f', 'Interworking, unspecified'), 401:('39', 'Bearer capability not authorized'), \
402:('15', 'Call rejected'), 403:('39', 'Bearer capability not authorized'), 404:('1', 'Unallocated number'), \
@@ -130,6 +130,12 @@
delay = self.cTime - self.iTime
connected = True
if not(self.ms_precision):
+ if isinstance(duration, MonoTime):
+ duration = duration.monot
+
+ if isinstance(delay, MonoTime):
+ delay = delay.monot
+
duration = round(duration)
delay = round(delay)
attributes = self._attributes[:]
@@ -167,6 +173,9 @@
self.global_config['_radius_client'].do_acct(attributes, self._process_result, self.sip_cid, time())
def ftime(self, t):
+ if isinstance(t, MonoTime):
+ return t.frtime()
+
gt = gmtime(t)
day = strftime('%d', gt)
if day[0] == '0':
I am still not exactly sure, if there are other places where floats are being expected and MonoTime is returned. I made a few test calls and they worked all the way, (both authorization and accounting) but I haven't spend that much time on the code to be 100% sure. If you feel that this patch is enough I am happy to provide you with a PR |
|
@twmobius thanks, that's sufficient amount of information for now. I am working on a reproducing the issue here: https://travis-ci.com/github/sippy/voiptests/builds/192232824 Stay tuned. |
crash reported in the sippy/b2bua#36
|
@twmobius ideed the crash is easily reproducible: https://travis-ci.com/github/sippy/voiptests/jobs/405739060#L2371 Working on a fix. |
|
@sobomax great! Just keep in mind that the |
working properly with MonoTimes. Issue: #36
|
Sorry to bother you, but take the time to check also with |
crashing anymore after 60a0fe49870437c, but does it actually work? Will find out soon if test fails.
|
@sobomax I've tried with all three routing responses I've mentioned before and it works properly every time. The timers in the Accounting Stop appear also to be ok. Thank you very much!! For reference: The only other traceback that I've been getting, but its unrelated to this ticket, is actually the one reported on #30 and it's the following: As suggested in #30 I've modified MsgBody.py as such: https://ubuntu.com/livepatch
diff --git a/sippy/MsgBody.py b/sippy/MsgBody.py
index 0f9b9c7..56286fb 100644
--- a/sippy/MsgBody.py
+++ b/sippy/MsgBody.py
@@ -59,6 +59,9 @@ class MsgBody(object):
def localStr(self, local_addr = None, local_port = None):
if type(self.content) == str:
return self.content
+ elif isinstance(self.content, unicode):
+ return str(self.content)
+
return self.content.localStr(local_addr, local_port)
def getType(self):to get around this. |
I've been trying to use sippy with radius support but I am facing an issue with MonoTime implementation.
It seems that if a route returned from radius which contains
credit-time,expiresandnp_expiresparameters those are being cast asints and when passed to aMonoAbsTimeoutobject, b2bua throws an exception thatmtime is not MonoTime. I am guessing similar issues might exist withmax_credit_timeand similar global variables.b2bua/sippy/Time/Timeout.py
Line 45 in 8adb5f9
I've thought of doing something simple like
but it didn't work. I'm trying to resolve this myself, but I haven't yet understood the source code with regards to Time objects and any help would be greatly appreciated.
@sobomax Thank you for the work you've done on Sippy!
The text was updated successfully, but these errors were encountered: