Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix the issue #136 across all backends. #156

Closed
wants to merge 4 commits into from
Closed

Fix the issue #136 across all backends. #156

wants to merge 4 commits into from

Conversation

ricardofandrade
Copy link
Contributor

See issue #136

The issue is cause because the session backend gets deleted during reconnect.
This solution simply don't do that.
With this fix session backends gained support for open connections and close them (via clean_up).
Also, it's possible to check if session backend is opened though in very simplistic way that just works. That can be improved in later versions.

No public API was changed and I tried to be conservative as possible with the backend objects.

This is a reasonable complex issue, so reviews are welcome.

I'm targeting soci 3.2.2 with fix pull request.

Please note DB2 backend depend (lightly) on #146 to work properly.
Other small DB2 fix is included in this request.

@ricardofandrade
Don't throw exception from cleanup once it gets called by a destructor.
9acc435
@ricardofandrade
Be sure calling close/reconnect doesn't make existing statements inva…
e38e725 
…lid.
@ricardofandrade
To compile on Windows
fba3de2
@ricardofandrade
Fixes a potential crash after using session::reconnect().
a4948c8 
In this case all existing objects referencing the session have dangling references
(or they may be pointing to another object because it coincidently is in the same memory address).
The fix: simply don't delete the session backend. Implement reconnection support to it.
It leads to unusable but valid objects (statement, blob, etc.), exceptions are expected.
@ghost ghost assigned mloskot Jun 3, 2013
@mloskot
Copy link
Contributor

mloskot commented Jun 23, 2013

I'd prefer if there is some feedback received from other developers, if they spot any potential issues for backends they maintain. There also seem there was no consensus from the discussion on #136.

This is the only pull request I haven't merged into hotfix/3.2.2 branch yet.

@ricardofandrade
Copy link
Contributor Author

I agree that some review and different opinions are necessary and really welcome.

One point I haven't mentioned is that this pull request doesn't aim to fix the case where the session goes out of scope before a dependent statement lying around. As a library developers we should protect the user against misuses like this, but I consider this situation out of the scope of #136 and something that the average SOCI user will avoid to do.
Fixing it would require a even bigger effort and maybe a change in SOCI's "public" API.

My goal here is really making session::reconnect() (as well the open()/close() pair) to not crash while the user is working with statements, what is a fair use of the existing features.

@mloskot
Copy link
Contributor

mloskot commented Sep 2, 2013

FYI, not merging this into 3.2.2

@mloskot mloskot mentioned this pull request Sep 2, 2013
Open
@mloskot mloskot closed this Apr 12, 2015
@mloskot mloskot mentioned this pull request Apr 12, 2015
Closed
@mloskot mloskot reopened this Apr 12, 2015
vadz added a commit to vadz/soci that referenced this pull request Jul 19, 2017
@vadz
Disable MySQL unit test currently always resulting in a crash
90a6db8 
Currently prepared statements do _not_ survive session::reconnect() due
to the issue SOCI#136, so running this test simply results in crashes.

Disable it for now, it should be reenabled when SOCI#156 or another solution
is applied.
@vadz vadz mentioned this pull request Jul 19, 2017
Closed
vadz added a commit that referenced this pull request Jul 19, 2017
@vadz
Disable MySQL unit test currently always resulting in a crash
d7c986c 
Currently prepared statements do _not_ survive session::reconnect() due
to the issue #136, so running this test simply results in crashes.

Disable it for now, it should be reenabled when #156 or another solution
is applied.
@mloskot mloskot added Status/Need-Feedback Need more info from OP Version/3.x labels Sep 23, 2017
@mloskot
Copy link
Contributor

mloskot commented Sep 23, 2017

This has been dangling for long time w/o any move forward. It also seems to target SOCI 3.x.

@vadz If you don't mind, I'm going to close this PR as not merged or partially merged.

@vadz
Copy link
Member

vadz commented Apr 6, 2021

Closing as suggested 3.5 years ago as this is not applicable any longer.

@vadz vadz closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mloskot mloskot

Labels
DB2 Firebird MySQL ODBC Oracle PostgreSQL SQLite Status/Need-Feedback Need more info from OP Version/3.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ricardofandrade @mloskot @vadz