Skip to content

Conversation

@mkramar
Copy link
Contributor

@mkramar mkramar commented Mar 20, 2013

DefaultValueMapper did not dispose OracleBlob objects which resulted in
memory leak

DefaultValueMapper did not dispose OracleBlob objects which resulted in
memory leak
@igor-tkachev
Copy link
Owner

Does it fix anything?

dest.SetValue(destObject, destIndex, source.GetValue(sourceObject, sourceIndex));

This line copes values. It does not create any new objects that need to be disposed.

@igor-tkachev
Copy link
Owner

Should I close this request?

@mkramar
Copy link
Contributor Author

mkramar commented Apr 4, 2013

True, it does not create any new objects.
However disposable objects are created by oracle provider and not disposed by BlToolkit.
This leads to memory leak.

As I said, in my experience reading large table with blobs resulted in out-of-memory exception.
After applying my fix the problem was fixed.
I think this fix should be merged into main.

@igor-tkachev
Copy link
Owner

The existing code just takes a value from data reader and pass it to object. So, the value belongs to the object and, I think, the object should take care of that value. If mapper disposes values then object will have a disposed value and it may work incorrectly if the value releases some sensitive data.

@igor-tkachev
Copy link
Owner

I understand that it works in your case, but it works accidentally. In common case this fix may cause a problem.

@mkramar
Copy link
Contributor Author

mkramar commented Apr 7, 2013

The client gets array of bytes, not OracleBlob object.Therefore the client cannot dispose it. OracleBlob is only obtained internally inside BlToolkit and converted into array of bytes.

The problem exists and needs to be fixed. My fix may not be optimal. Potentially it can break something in different scenarious. I guess you just have to test it.

@igor-tkachev
Copy link
Owner

I fixed Oracle data provider. Please let me know if it does not solve the problem.

@mkramar
Copy link
Contributor Author

mkramar commented Apr 7, 2013

Your fix looks better than mine :) I'll give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants