Skip to content

#1017 Add a finalizer SmartSubtransportRegistration - #1220

Closed
rooflz wants to merge 1 commit into
libgit2:vNextfrom
rooflz:vNext
Closed

#1017 Add a finalizer SmartSubtransportRegistration#1220
rooflz wants to merge 1 commit into
libgit2:vNextfrom
rooflz:vNext

Conversation

@rooflz

@rooflz rooflz commented Oct 30, 2015

Copy link
Copy Markdown
Contributor

-Implement IDisposable and added finalizer to
SmartSubtransportRegistration
-Updated tests to now call dispose
-Removed CannotUnregisterTwice test, as this test is no longer
applicable after implementing dispose pattern.

@ethomson

Copy link
Copy Markdown
Member

Ignore that last comment - I thought this was IDisposeable on the stream, not the registration.

@jamill

jamill commented Oct 30, 2015

Copy link
Copy Markdown
Member

Just curious - from the comments in #1017, it looks like the direction was to not make the registration disposable?

@rooflz

rooflz commented Oct 30, 2015

Copy link
Copy Markdown
Contributor Author

Need to work on my reading comprehension. Looks like the direction was to add a finalizer, and not make the object disposable. Is there a downside to also making it disposable and have the finalizer call dispose? If the object is not made disposable then we no longer have control over when things are cleaned up and must wait for the finalizer. If more tests are added then they must ensure there are no naming collisions, which makes things a bit awkward in my opinion. Will be happy to remove the dispose pieces though if that is the consensus.

Speaking of tests...looks like I've broken everything.

@nulltoken

Copy link
Copy Markdown
Member

Another approach would be to mimic what's has been done for OdbBackend.

  • Make the Free method test if the type is IDisposable. When that's the case, invoke the Dispose() method. This would allow the user to derive from the core type, and implement IDisposable by himself/herself when requiring some cleanup.
  • This would require the core type to not be sealed any longer (@ethomson any potential drawback here?)

See this unit test that asserts this feature.

SmartSubtransportRegistration
-Updated tests to now call dispose
-Removed CannotUnregisterTwice test, as this test is no longer
applicable after implementing dispose pattern.
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.

5 participants