C# binding appears unstable -- the dynamic extent of the delgates passed to unmanged code seems the culprit

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Run the base example any -- it will die after a few queries to any
set_uri_callback.
2.
3.

What is the expected output? What do you see instead?

What version of the product are you using? On what operating system?
2.8 (with the 2.9 .cs changes).  On both Windows XP and Vista.

Please provide any additional information below.

I've gotten stability.  Note the problem from: 
http://msdn.microsoft.com/en-us/library/7esfatk4.aspx
A reference to the function pointer to a managed delegate held by unmanaged
code does not prevent the common language runtime from performing garbage
collection on the managed object. 

This is precisely what the current binding code does.  To fix see diff
file.  Basically, I just store away the callback passed into unmanaged code
in the managed code (using an event).

I've also noticed some HeapFrees on unAlloc'ed items (using depends.exe). 
To clear these, I've:
1) added, what should be unnecessary [In] direction setting to all passed
parameters, and
2) bizarrely, I've add to change the long to an IntPtr remote_ip in the
MongooseRequestInfo structure.  This doesn't make sense, but without it
under Vista (framework 3.5) this structure wasn't marshaled properly. 

Note: you can go back to string types on the http_headers doing this,
though there is really no reason to marshal these anyway..

--Steve
Stephen.P.Smith"AT"Intel.com

Original issue reported on code.google.com by spso...@gmail.com on 29 Aug 2009 at 3:11

Attachments:

• foo.txt

[GoogleCodeExporter]

Thank you!
I have granted you commit access, would you mind committing your code yourself, 
please?

Also I have a question about the calling convention.
Recently I have changed the Makefile by adding "/Gz" flag which made all 
exports be
_stdcall: http://code.google.com/p/mongoose/source/detail?r=467 (Reported bug
http://code.google.com/p/mongoose/issues/detail?id=81&can=1)
Should DllImports in mongoose.cs be changed from cdecl to stdcall (or maybe 
calling
convention attribute must be dropped? Or is it safe to continue using cdecl
convention and revert change 467?

Original comment by valenok on 29 Aug 2009 at 1:10

[GoogleCodeExporter]

Yeah, please add the comment explaining why "delegates" event is needed.

Original comment by valenok on 29 Aug 2009 at 1:14

[GoogleCodeExporter]

Comment1: I put the code in --probably early next week. I want to run a few more
experiments.  Comment 2: The event and saving the callback func can drop in as 
change
by itself, which I will document.

Re: calling convention.  I think either convention is *supposed* to work.  Let 
me run
some quick & dirty experiments.  Right now I've got cdecl in the calling 
convention
and the .dll compiled without the /Gz and everything appears ok. Did you find & 
url
references for the comment in Issue 81, re "Various parts of MSDN note that a
callback from C must be __stdcall to work with the delegate marshalling code."??

BTW. on my vista machine, one definite bug comes out for me is that "long" in 
.net is
int64 but "long" generally on x86 c and in _mongoose.dll is int32.  This makes 
every
call to marshal the MongooseRequestInfo structure incorrect because of the 
remote_ip
long.  Declaring it "int" in the .cs file works.  But this is a strange 
mismatch and
the it supposed to be handled correct, I don't believe I saw it on an older XP
machine at work, so it may be some weird setup I have at home (some beta Visual
Studio stuff).

Original comment by spso...@gmail.com on 30 Aug 2009 at 5:40

[GoogleCodeExporter]

Great, thanks.
Re: Issue 81. What I did find is that __stdcall is a requirement for VB. VB 
cannot
interop with __cdecl DDLs. People have managed to create bizarre cdecl -> 
stdcall
bridges to make __cdecl DLLs work in VB.
Python's ctype documentation mentions __cdecl convention.

Original comment by valenok on 30 Aug 2009 at 9:09

[GoogleCodeExporter]

here's the best URL for the MSDN __stdcall requirement I could find:

http://msdn.microsoft.com/en-us/library/aa288468(VS.71).aspx

Of course, since the vast majority of Windows API boundaries are __stdcall (and 
this is hidden away in a macro) I 
would assume most Microsoft documentation folk wouldn't notice it is clearly a 
requirement.

Original comment by jhmcale...@gmail.com on 30 Aug 2009 at 11:11

[GoogleCodeExporter]

On the issue of __stdcall, by convention on windows the API entries for 
mongoose for a dll should also be 
__stdcall. I did not report this as a defect, however, because it is clear that 
the c# runtime can cope with cdecl 
apis.

I'm unaware of any formal *requirement* for the apis (mg_start, etc) to be 
__stdcall, merely long cultural tradition 
on Windows.

Original comment by jhmcale...@gmail.com on 30 Aug 2009 at 11:13

[GoogleCodeExporter]

On http://msdn.microsoft.com/en-us/library/aa288468(VS.71).aspx they show an 
example
with __stdcall, that's right, but they do not mention that __stdcall for 
callbacks is
a requirement.

What would be the easiest way to prove or disprove that __stdcall is a 
requierement?

Original comment by valenok on 30 Aug 2009 at 12:37

[GoogleCodeExporter]

I agree that the cited page does not state it is a requirement. That was why I 
didn't post it on the original 
defect.

However:

 - It posts an example known to work
 - There appears to be no syntax to select (from c#) what calling convention a marshalled delegate should 
use.

Given these two facts, my personal conclusion was that __stdcall was a 
requirement. Given the historical 
pervasiveness of this on the Windows platform, I have not investigated further.

Original comment by jhmcale...@gmail.com on 30 Aug 2009 at 5:24

[GoogleCodeExporter]

Can callback delegate be declared as __cdecl, I wonder?

Original comment by valenok on 30 Aug 2009 at 7:10

[GoogleCodeExporter]

Hmm, a quick test shows either cdecl or stdcall "appears" to work, but your 
right its
hard to find a subtle bug.  

It does appear that stdcall is more "standard" :-) so perhaps that should be the
default.  Note that documentation for the standard marshal code,
Marshal.GetFunctionPointerForDelegate Method only mentions stdcall.

@comment9: Yes you can attribute delegates with calling protocol.  
[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
public delegate void MongooseCallback2([In] IntPtr conn, [In]ref 
MongooseRequestInfo
ri, [In]IntPtr user_data);

I suggest keeping patch for issue 81 and leaving everything stdcall.  I think 
that is
the default but I'll decorate all the dllimports and the delegate with the 
calling
convection attribute anyway. I'll stick that in the .cs file.  

Original comment by spso...@gmail.com on 30 Aug 2009 at 9:28

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Purpose of code changes..
- Fix for issue 85

When reviewing my code changes, please focus on:
- Added events and pushed delegates onto them before calls to unmanaged code
- Added cdecl spec for delegate passed into unmanaged code
- Added additional url example in example.cs which gives all ri info

Additionally, fixed the "long" (64bit) versus "int" (32bit) in remote_ip.

I've put changes directly in trunk.   

--steve

Original comment by spso...@gmail.com on 30 Aug 2009 at 11:18

[GoogleCodeExporter]

Aha - I stand corrected. If annotating the function pointer with __cdecl works, 
I see no particular reason to 
require __stdcall, apart from long cultural tradition.

Windows users will certainly expect to see __stdcall APIs, but if they aren't 
needed, (and the c# is simply declared 
to cope), then perhaps they should be left out.

Original comment by jhmcale...@gmail.com on 31 Aug 2009 at 10:03

[GoogleCodeExporter]

I would be slow moving everything to __stdcall, since at least python ctypes 
module 
mentions __cdecl and is expecting __cdecl. I did not verify how python works 
with 
__stdcall exports.

Original comment by valenok on 31 Aug 2009 at 11:31

[GoogleCodeExporter]

John, would you mind trying spsone1's changes in your environment, please?

Original comment by valenok on 31 Aug 2009 at 3:37

[GoogleCodeExporter]

I have to discontinue C# binding support - I don't have resources for it at the 
moment.

Original comment by valenok on 7 Sep 2010 at 8:44

• Changed state: WontFix