Monobjc / monobjc

Git Repository for the Monobjc Project
http://www.monobjc.net/
17 stars 3 forks source link

Fix multi-domain and multi-threading issues. #405

Closed aarononeal closed 11 years ago

aarononeal commented 11 years ago

This commit addresses domain and concurrency issues by implementing additional locks, error checking, and domain name mangling.

Support multiple app domains by appending 'Domain[Id]' to all .NET defined types.

letiemble commented 11 years ago

Well done. It looks much nicer that the previous domain-switching work-around.

letiemble commented 11 years ago

In order to make the review easier, I have created a pull request (monobjc#424) that only takes the multi-threading fix. For the multi-domain part, I would like to make some experiments with another approach.

letiemble commented 11 years ago

I have started to review the multi-domain part of your patch and I wonder if it is better to start with multi-domain enabled or disabled ? You also mention in an email that you may have a plan to make the first domain use direct naming and the others use a suffixed naming. Is it still doable ?

aarononeal commented 11 years ago

On the multi-domain default, if it weren't for the name mangling it wouldn't be an issue, the default could always be on and we wouldn't need a switch. So, it comes down to whether you prefer support for multi-domains or non-mangled names. Generally I don't think someone will be writing code where the name mangling matters, so that's why I set the default to on, but either way is fine with me.

As for making the first domain use direct naming, I wasn't able to achieve this because:

  1. Lookup is performed on the non-mangled name since there might be a native type.
  2. Only if that fails is a new managed type registered using the domain mangled name.
  3. If domain #1 registered a non-managled name, domain #2 would find it and assume it was native and safe to bind when it was not.

There are various ways to solve that, but none of them were simple enough to incorporate into that already large patch.

Not solved in the domain work I submitted, but in another commit I have on this side, I also fixed a case where domain IDs get recycled. That is, domain #1 is up, domain #2 is up, domain #2 gets shutdown, then domain #2 starts up again. Types have to be allowed to rebind in that situation.

Finally, there is a related issue which I have solved in another patch (also not yet submitted) which allows me to distinguish which types are supposed to be native backed and which aren't, so if I were to submit that patch, I could most likely use it to resolve the name mangling issue in the first domain. But, it requires that we solve those extraneous _Definitions types I mentioned to you via e-mail.

To sum up, I propose the following:

  1. You take a look at fixing the _Definitions types that don't belong.
  2. I'll create a new branch and rebase the domain work.
  3. I'll add the domain recycling fix.
  4. I'll add the patch which flags native types and does not register a managed type if missing. This also fixes Class.Get() to properly return null when the native type isn't available and fixes some tests. It requires the invalid _Definitions go away.
  5. Once all that is in place I can take another look at how to remove name mangling on domain #1 and therefore remove the switch to enable/disable multi-domain support (because it can always be on). But, even with everything else in place, avoiding the mangling may be hard to achieve.
  6. I'll submit a new pull with the fixes.
letiemble commented 11 years ago

I have a pretty big concern about name mangling: it may not work when using NSCoder API and especially when loading NIB: the deserialization process lookup class by name. So if class names are mangled, custom classes (i.e. NSView subclasses) may not be found. Any thoughts ?

aarononeal commented 11 years ago

Without better options, I think it comes down to picking which limitations are easier to live with. :-)

The limitation of this approach is that NSCoder resources based on managed code types will have to be tied to a domain or late-bound at the owner level. That's a pretty severe limitation, but may be a reasonable compromise for secondary domains depending on the scenario. If we allow domains to be bootstrapped with a caller specified token then any NSCoder resources can be targeted by appending the token. Ideally, the primary domain (or one of the caller's choosing) can have no token and thus no name mangling.

If we instead allow domains to resolve to types registered by other domains (as the code currently does), we must continue to maintain separate instance caches. This means we will have that data sharing issue when looking up a managed type with any state because that state is only valid in the other domain (unless marshalled using IVars).

The real problem with cross-domain type sharing is how this manifests in callbacks because they occur in the domain where the proxy was registered, not where the object instance was created. We would need to come up with a way to have the proxies invoke their delegate on the instance domain. My earlier NSThreadLauncher approach solved it for that specific type by handling it in the callback itself, but we really need a more general purpose solution than that. Then, even with that solved, we would still need to handle domain unloading.

aarononeal commented 11 years ago

I'm going to close this pull and add a new one for review.