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.

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

The Framework must not recursively call ServiceFactory::GetService and ServiceFactory::UngetService on the same thread for the same bundle #213

Closed
jeffdiclemente opened this Issue Jun 9, 2017 · 6 comments

Comments

Projects
2 participants
@jeffdiclemente
Contributor

jeffdiclemente commented Jun 9, 2017

According to the OSGi spec for a ServiceFactory, the getService(Bundle bundle,ServiceRegistration<S> registration) method states:

If this method is recursively called for the specified bundle, a
framework event of type FrameworkEvent.ERROR is fired containing a service exception of type
ServiceException.FACTORY_RECURSION and null is returned to the bundle.

The Framework currently doesn't enforce this. This issue is to track the implementation of this constraint.

@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente Jun 9, 2017

Contributor

Sascha described in #202 how this behavior could happen:

When the framework calls GetService, it effectively calls into external code and the service factory implementation could (in theory) call GetService or UngetService on the same factory instance again (either explicitly or implicitly via calling back into the framwork API). This needs to be detected and handled.

Test code needs to be written to trigger this behavior and verify the implementation fixes the issue.

Contributor

jeffdiclemente commented Jun 9, 2017

Sascha described in #202 how this behavior could happen:

When the framework calls GetService, it effectively calls into external code and the service factory implementation could (in theory) call GetService or UngetService on the same factory instance again (either explicitly or implicitly via calling back into the framwork API). This needs to be detected and handled.

Test code needs to be written to trigger this behavior and verify the implementation fixes the issue.

@saschazelzer

This comment has been minimized.

Show comment
Hide comment
@saschazelzer

saschazelzer Jun 28, 2017

Member

I am reading paragraph 10.1.25.1 of the OSGi Core 6.0 specs about service factories. The specified error handling is not in line with our code in ServiceReferenceBasePrivate::GetServiceFromFactory and I am thinking about using the correct FrameworkEvent::Type enum (error instead of warning) and using our existing ServiceException class in the created framework event. Do you disagree?

Member

saschazelzer commented Jun 28, 2017

I am reading paragraph 10.1.25.1 of the OSGi Core 6.0 specs about service factories. The specified error handling is not in line with our code in ServiceReferenceBasePrivate::GetServiceFromFactory and I am thinking about using the correct FrameworkEvent::Type enum (error instead of warning) and using our existing ServiceException class in the created framework event. Do you disagree?

@saschazelzer saschazelzer moved this from Planned to In Progress in Release 3.2 Jun 28, 2017

@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente Jun 29, 2017

Contributor

That all sounds good to me. I noticed that code coverage for the ServiceException is low and this is a great opportunity to increase it.

It does beg the question on whether we should implement the BundleException class and use it consistently.

Also, I think that the work to convert over to consistently using ServiceException should be a separate issue independent of this one.

Contributor

jeffdiclemente commented Jun 29, 2017

That all sounds good to me. I noticed that code coverage for the ServiceException is low and this is a great opportunity to increase it.

It does beg the question on whether we should implement the BundleException class and use it consistently.

Also, I think that the work to convert over to consistently using ServiceException should be a separate issue independent of this one.

@saschazelzer

This comment has been minimized.

Show comment
Hide comment
@saschazelzer

saschazelzer Jun 29, 2017

Member

Good points. I think we should implement BundleException and use it consistently, ideally in 3.x in a backwards compatible manner. I am going to create new issues.

Member

saschazelzer commented Jun 29, 2017

Good points. I think we should implement BundleException and use it consistently, ideally in 3.x in a backwards compatible manner. I am going to create new issues.

@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente Jun 29, 2017

Contributor

I agree on using BundleExceptions consistently and in a backwards compatible way for 3.x.

Thanks!

Contributor

jeffdiclemente commented Jun 29, 2017

I agree on using BundleExceptions consistently and in a backwards compatible way for 3.x.

Thanks!

saschazelzer added a commit that referenced this issue Jun 30, 2017

saschazelzer added a commit that referenced this issue Jul 13, 2017

saschazelzer added a commit that referenced this issue Jul 18, 2017

saschazelzer added a commit that referenced this issue Jul 25, 2017

@saschazelzer

This comment has been minimized.

Show comment
Hide comment
@saschazelzer

saschazelzer Aug 14, 2017

Member

The implementation in #222 makes use of a thread-local variable. Segmentation faults of tests from builds based on MingGW revealed a problem with the TLS implementation in libwinpthreads, which is also noted here:

Alexpux/MINGW-packages#2519
https://sourceforge.net/p/mingw-w64/bugs/527/

Therefore, the checks for recursive service factory calls are going to be disabled for MinGW platforms until this is resolved.

Member

saschazelzer commented Aug 14, 2017

The implementation in #222 makes use of a thread-local variable. Segmentation faults of tests from builds based on MingGW revealed a problem with the TLS implementation in libwinpthreads, which is also noted here:

Alexpux/MINGW-packages#2519
https://sourceforge.net/p/mingw-w64/bugs/527/

Therefore, the checks for recursive service factory calls are going to be disabled for MinGW platforms until this is resolved.

saschazelzer added a commit that referenced this issue Aug 14, 2017

@saschazelzer saschazelzer moved this from In Progress to Done in Release 3.2 Aug 17, 2017

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