On Tue, 2004-12-21 at 10:55 +0000, Robert Shearman wrote:
And those same users would have been inconvenienced with the 20041201 release. It wouldn't crash, but it's likely the program still wouldn't work.
Well, I think an engineering goal for us should be that WineHQ CVS is at all times:
a) Internally consistent b) Working (even if it's not perfect)
This is good for several reasons. Obviously it helps users, but it also helps us: more people can help out, whereas if the code is missing pieces only we know about anybody who is interested in coding alongside us will fall at the first hurdle (I was talking to somebody who seemed to know DCOM pretty well in IRC last night, no idea if he'll turn up and help but you never know).
It helps in another way: it's easier to spot regressions if we have a baseline of "it works". That way when it stops working, we can figure out exactly which patch did it quickly. If the code simply crashes within a few yards of the first CoMarshalInterface call because we didn't put in the 6 lines of code necessary to keep it working during the transition, we have no idea if the code that comes later really works.
If we're not going to try and keep CVS working then we shouldn't be submitting patches there, especially if native DCOM is busted. We should be working in a private arch or subversion/svk branch where it doesn't matter if we go a year with no InstallShield support.
I'll see what I can come up with this week. I know you have some work on the stub manager outstanding and I think it would be fairly easy to build on that work to implement the table marshaling.
OK, you have the latest patch in your inbox. It's different from the previous but not much. If you think it's OK I'll go ahead and submit it (and I'll check it still works with InstallShield too).
Once that patch is in you're right, table marshalling should be easy. At that point we can start to remove some of the extra threads (there are 3 helper threads used currently, right?). The ones that are left will still need to join the marshalling apartment though, so this patch will still be necessary.
I think we discussed about using a field in the RPCMESSAGE reserved for the RPC runtime (which is what we are emulating here). I don't think any other reworking is required.
I couldn't see at the time how that'd work: the problem is we need to keep track of information across arbitrary application stack frames, ie
IRpcStubBuffer::Invoke -> InstallShield code -> proxy call -> pipe?!?
We can't transfer information across the InstallShield code without using a TLS slot. In native DCOM this doesn't matter as the proxy doesn't need to use the same pipe that the RPC came in from, it'll be using a different connection. In our current code, that won't work as the original caller can only re-enter from RPCs incoming on the same pipe it originally called out on. Native RPC doesn't use a thread per stub manager, and each outgoing call that could re-enter spawns another thread that dispatches incoming RPCs ad-infinitum.
Ugh, that's clear as mud I know. The issue is that our threading is wrong: we need more instances of the threads so re-entrancy works correctly. Before we change anything in this area then we need:
a) the unit tests you wrote to be in CVS b) more tests for out of process COM and out of process call re-entrancy
Once we can check the state of the code with more than just the random InstallShields we have, then I think it'll be OK to rework the threading/pipe management so thread-affinity works correctly.
The main changes I object to are adding more fields to structures that I was previously pruning of hacks.
While we have any helper threads that don't correspond exactly to what native DCOM does they'll need to enter the original apartment, I don't see any way around that. Simply not making them join will mean the current code is not only hacky but also wrong, which goes back to the first point. During the transition to real DCOM some hacks will be needed to support the original infrastructure, it's unavoidable. They can and will be removed as the rest of the code allows it.
We're both keen to get correct code as soon as possible, but this is a long term project and we want to keep disruption to a minimum.
thanks -mike