Opened 8 years ago

Closed 8 years ago

#1253 closed task (complete)

Datasource refactor finishing touch 3: factory/containers and wrappers

Reported by: jelte Owned by: jelte
Priority: low Milestone: Sprint-20111011
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The datasource python wrappers (#1179) and several other tasks were developed simultaneously, so any updates to the API may not be reflected in the wrappers yet.

This is one of the tasks to finish that up, most of these should be fairly small or even trivial.

ticket #1206 added a container class as a form of factory function that need a wrapper. (Alternatively, since 1206 has not even been submitted for review yet, and its interface may change, whatever it becomes needs to be reflected in the wrappers)

Subtickets

Change History (14)

comment:1 Changed 8 years ago by jelte

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

comment:2 Changed 8 years ago by jelte

  • Priority changed from major to minor

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 3

comment:4 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:5 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review.

Instead of wrapping the factory classes themselves, I decided to simply make the python isc.datasrc.Client class wrap the DataSourceContainer? instead of the DataSource? directly. It then calls the needed functions on the contained datasource instance instead of through a separate python class.

Most of the changes however are other issues this change revealed;

  • I added an optional parameter to various createXXX() functions; it now takes a PyObject?*, and if not NULL, it'll INCREF the object, and DECREF it upon destruction. This way we can make sure any object that the object created by createXXX needs to be alive during its lifetime will be. (in this case we need Client not to be destroyed before ZoneIterator?, ZoneUpdater?, or ZoneFinder? are)
  • I've added a few catches in factory.cc's create() call; they simply copy the exception and reraise it. There is a problem if we don't do so, if the creation fails because of one of those exceptions, the loaded library may be unloaded, and the stack unroll loop may then want to look into it to see what the exception is. By copying it outside of the dynlib we should be able to avoid this problem. This may mean we need to strictly define which exceptions can be raised (or maybe i'm just overseeing something here).

Oh and one additional comment; i've made the datasource config parameter of the lowest-common-denominator type; i.e. a string containing a JSON representation. We may want to add more advanced types in the future but this should be usable enough for the near future.

comment:6 Changed 8 years ago by vorner

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

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

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

Hello

First, I fixed a problem in Makefile, it failed with make -j6 here (as the library was not yet made).

Secondly, I still have trouble with it, it fails the tests:

Running test: datasrc_test.py
.EEE.EE
======================================================================
ERROR: test_constructors (__main__.DataSrcClient)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/python/isc/datasrc/tests/datasrc_test.py", line 74, in test_constructors
    isc.datasrc.DataSourceClient, "sqlite3", "{}")
  File "/usr/lib64/python3.2/unittest/case.py", line 557, in assertRaises
    callableObj(*args, **kwargs)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:/home/vorner/testing/bind10/lib/sqlite3_ds.so: undefined symbol: _ZNK3isc9Exception4whatEv

======================================================================
ERROR: test_find (__main__.DataSrcClient)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/python/isc/datasrc/tests/datasrc_test.py", line 194, in test_find
    dsc = isc.datasrc.DataSourceClient("sqlite3", READ_ZONE_DB_CONFIG)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:/home/vorner/testing/bind10/lib/sqlite3_ds.so: undefined symbol: _ZNK3isc9Exception4whatEv

======================================================================
ERROR: test_iterate (__main__.DataSrcClient)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/python/isc/datasrc/tests/datasrc_test.py", line 83, in test_iterate
    dsc = isc.datasrc.DataSourceClient("sqlite3", READ_ZONE_DB_CONFIG)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:/home/vorner/testing/bind10/lib/sqlite3_ds.so: undefined symbol: _ZNK3isc9Exception4whatEv

======================================================================
ERROR: test_update_delete_abort (__main__.DataSrcUpdater)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/python/isc/datasrc/tests/datasrc_test.py", line 352, in test_update_delete_abort
    dsc = isc.datasrc.DataSourceClient("sqlite3", WRITE_ZONE_DB_CONFIG)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:/home/vorner/testing/bind10/lib/sqlite3_ds.so: undefined symbol: _ZNK3isc9Exception4whatEv

======================================================================
ERROR: test_update_delete_commit (__main__.DataSrcUpdater)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/python/isc/datasrc/tests/datasrc_test.py", line 278, in test_update_delete_commit
    dsc = isc.datasrc.DataSourceClient("sqlite3", WRITE_ZONE_DB_CONFIG)
isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:/home/vorner/testing/bind10/lib/sqlite3_ds.so: undefined symbol: _ZNK3isc9Exception4whatEv

----------------------------------------------------------------------
Ran 7 tests in 0.003s

FAILED (errors=5)
make[7]: *** [check-local] Error 1
make[7]: Leaving directory `/home/vorner/work/bind10/src/lib/python/isc/datasrc/tests'

I have a faint memory that gentoo linker is set to strip symbols that are not used when loading dynamic libraries or something and maybe the python test doesn't use the symbol, so it gets lost or something. But that's just a wild guess.

The stuff in a8a8ceb589f9f3bf4da29717eec446cb2766032c looks strange. What happened when it threw? Because this is both incomplete (there's infinitely more unhandled exceptions), wrong (as something that inherits from DataSourceError? can be thrown, in which case this loses information) and woodoo-looking.

I'm thinking if we except our iterators, etc, to hold a shared pointer to the parent data source client. Maybe not, so in which case your approach might be nice (because python programmers don't expect such attacks from the language, C++ ones are already used).

And a question, does anything use the factory in some way already? Or we need to update different branches?

Thanks

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

  • Owner changed from jelte to vorner

Replying to vorner:

Hello

First, I fixed a problem in Makefile, it failed with make -j6 here (as the library was not yet made).

Thanks

Secondly, I still have trouble with it, it fails the tests:

isc.datasrc.Error: Failed to create DataSourceClient of type sqlite3:/home/vorner/testing/bind10/lib/sqlite3_ds.so: undefined symbol: _ZNK3isc9Exception4whatEv
I have a faint memory that gentoo linker is set to strip symbols that are not used when loading dynamic libraries or something and maybe the python test doesn't use the symbol, so it gets lost or something. But that's just a wild guess.

There is a more general issue that python does weird stuff to the symbol tables when it loads libraries.
Does the sqlite3_ds.so end up being linked to libcc? I did find another problem where on solaris it couldn't find the .so in the first place.

The stuff in a8a8ceb589f9f3bf4da29717eec446cb2766032c looks strange. What happened when it threw? Because this is both incomplete (there's infinitely more unhandled exceptions), wrong (as something that inherits from DataSourceError? can be thrown, in which case this loses information) and woodoo-looking.

The problem is that if a dlopened() module throws an exception, and that exception is (partly) defined within that library (including exception subclasses that don't override all methods of their parents, whether or not these subclasses are defined outside the lib itself), by the time the exception handler gets it, the library may be unloaded already (and due to the nice and clean RAII pattern, this will always be the case if the constructor fails), and it will try to look up type information in a library that no longer exists, resulting in undefined behaviour (or most probably, segfaults when trying to read or destroy the exception object).

So that's why i said we need to restrict by contract which exceptions are thrown. But given some other issues i saw, perhaps it is much better to restrict it even further, and not let createInstance throw anything, ever.

So here's another approach; i've added a std:string& error argument to the createInstance prototype, and they may set some error to it and return NULL if there was a problem. The caller still has a high-level catch around it (a half-useless exception is better than a segfault with little info imo). Currently the createInstance, eh, instances don't make much difference in what they catch themselves, but we can always add those. And if the result is NULL, the caller will throw the exception.

This also meant there was no distinction between DataSourceConfigError? and DataSourceError? anymore, so i removed the first (we can use it again if we want to do config check *before* initialization)

Of course we *could* also use a global registry thingy and not unload any library until the very end of the whole program. But ew.

I'm thinking if we except our iterators, etc, to hold a shared pointer to the parent data source client. Maybe not, so in which case your approach might be nice (because python programmers don't expect such attacks from the language, C++ ones are already used).

And a question, does anything use the factory in some way already? Or we need to update different branches?

No, it's not used yet. AFAIK, only jinmei's current branch for 1261 uses a slightly different way of initializing the DataSourceClient? object, but that shouldn't be a big change (same info, just in a different object).

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

  • Owner changed from vorner to jelte

Hello

Replying to jelte:

There is a more general issue that python does weird stuff to the symbol tables when it loads libraries.
Does the sqlite3_ds.so end up being linked to libcc? I did find another problem where on solaris it couldn't find the .so in the first place.

I don't know, but it seems to work this time. Maybe it helped as well, for some reason.

So that's why i said we need to restrict by contract which exceptions are thrown. But given some other issues i saw, perhaps it is much better to restrict it even further, and not let createInstance throw anything, ever.

That might make sense. But in that case, shouldn't (...) be caught as well, just as std::exception? At last in the createInstance functions?

Thanks

comment:10 Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

good point, added.

comment:11 Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

Actually, I thought this one:

-    return (new InMemoryClient());
+    try {
+        return (new InMemoryClient());
+    } catch (const std::exception& exc) {
+        error = std::string("Error creating memory datasource: ") + exc.what();
+        return (NULL);
+    }

And corresponding one in the sqlite3 data source. This way the first catch (for std::exception) might want to check if the falling thing is descendand of std::exception or not, and that might be written in the already deleted data source. But maybe I'm too paranoid about it.

With regards

comment:12 Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

Actually, yeah, we should make sure that nothing escapes :) So I added catch(...) to the creates. I did leave it in the caller to create, just in case we or someone adds one that mucks it up :)

comment:13 Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

OK, thanks. Please, feel free to merge.

comment:14 Changed 8 years ago by jelte

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

Merged, closing.

Note: See TracTickets for help on using tickets.