Code Review: socket2

tfpt review /shelveset:socket2;REDMOND\jflam

Overall - looks good! We really want to turn on tests for this stuff.
Jim’s looking at getting the latest rubinius specs running so we can
deprecate our current spec snapshot soon - hopefully this week.

A few minor nits below:

Seems like you have a couple of redundant Protocols.CastToString calls
here - domain is always a MutableString based on your ctor …

    private static Socket CreateSocket(CodeContext/*!*/ context, 

MutableString/!/ domain, MutableString/!/ type, object/Numeric/
protocol) {
RubyExecutionContext ec =
RubyUtils.GetExecutionContext(context);
RubyClass rubySocketClass =
ec.GetClass(typeof(Ruby.StandardLibrary.RubySocket));

        AddressFamily addressFamily = 

(AddressFamily)RubyUtils.GetConstant(context, rubySocketClass,
SymbolTable.StringToId(Protocols.CastToString(context, domain)), true);
ProtocolType protocolType =
(ProtocolType)(Protocols.CastToFixnum(context, protocol));
SocketType socketType =
(SocketType)RubyUtils.GetConstant(context, rubySocketClass,
SymbolTable.StringToId(Protocols.CastToString(context, type)), true);
return new Socket(addressFamily, socketType, protocolType);
}

Also … Socket should be Socket/!/ since there is only one code path
out of this method barring exceptions. I’ve fixed these minor things in
my copy.

Following this analysis, this also leads to _socket in
RubyBasicSocket:23 being a /!/ as well.

BTW, I really like the /Numeric/ annotations that you’ve been putting
in your code. This could lead us to doing something smarter in the
future in the binder like adding a parameter attribute [Numeric] to
coerce the binder into doing some smarter things with the conversions.

Thinking aloud about this method - would it make more sense to have
MutableString and int overloads that delegate to helpers rather than
dropping everything into a single method like this one here?

    internal static MutableString 

ConvertToHostString(CodeContext/!/ context, object hostname) {
if (hostname == null) {
return null;
}
if (hostname is MutableString) {
MutableString strHostname = (MutableString)hostname;
// Special cases
if (strHostname == “” ) {
strHostname = new MutableString(“0.0.0.0”);
} else if (strHostname == “”) {
strHostname = new MutableString(“255.255.255.255”);
}
return strHostname;
}
int iHostname;
if (Protocols.IntegerAsFixnum(hostname, out iHostname)) {
// Ruby uses Little Endian whereas .NET uses Big Endian
IP values
byte[] bytes = new byte[4];
for (int i = 3; i >= 0; --i) {
bytes[i] = (byte)(iHostname & 0xff);
iHostname >>= 8;
}
return new MutableString(new
System.Net.IPAddress(bytes).ToString());
}
return Protocols.CastToString(context, hostname);
}

Some other points - seems some of the code below could be delegated to
the Protocols.CheckSafeLevel() helpers that you added. BTW, we should
probably spend some time and make a call about what we’re going to do
about all of the safe level stuff. Arguably this stuff isn’t necessary
due to CLR safety (especially in Silverlight contexts).

    internal static void CheckSecurity(CodeContext/*!*/ context, 

object self, string message) {
RubyExecutionContext ec =
RubyUtils.GetExecutionContext(context);
if (ec.CurrentSafeLevel >= 4 && ec.IsObjectTainted(self)) {
throw RubyExceptions.CreateSecurityError("Insecure: " +
message);
}
}

Thanks,
-John

Hi John,
What is the process of these code reviews of external contributions?
Are
you expecting to begin a dialogue, since you do ask questions throughout
the
review? Or is it more of a feedback opportunity to let us know what you
have done to the code? For instance, are you expecting me to make
changes
discussed below and resubmit the patch - clearly not in the case of the
first item as you have said that you changed it already on your local
copy?
Cheers,
Pete

Peter Bacon D.:

What is the process of these code reviews of external contributions?
Are you expecting to begin a dialogue, since you do ask questions
throughout the review? Or is it more of a feedback opportunity to let
us know what you have done to the code? For instance, are you
expecting me to make changes discussed below and resubmit the patch -
clearly not in the case of the first item as you have said that you
changed it already on your local copy?

Generally, if folks have opinions about the review either way, please
let us know.

Specifically on your contribution, I’m having a heck of a time getting
it to compile under Silverlight. I’m going to block off some more time
tomorrow to see what I can do. Sockets are only partially implemented in
Silverlight, so I have to figure out the delta. Don’t worry about making
changes - once I get it compiling under SL, I’ll check it in and you can
modify my changes :slight_smile:

Thanks,
-John

I see that the Ruby.StandardLibrary.RubySocket is implemented in C#.
Is it common to write the core in C#? Has anything been written in
Ruby itself?

Sorry for the newbie question… is there a FAQ for this type of
situation?

C.J. Adams-Collier:

I see that the Ruby.StandardLibrary.RubySocket is implemented in C#.
Is it common to write the core in C#? Has anything been written in
Ruby itself?

Our guideline is: if it was written in C in MRI, we’re writing in C#.

Thanks,
-John

The majority if not all of the IronRuby libraries have been written in
C# so
far. There have been discussions about this on the mailing list before.

There are a number of Ruby libraries that have been written in Ruby.
Once
IronRuby is fully compliant then it should be able to load these and run
them straight up.

Obviously stuff that cannot be written in Ruby (such as accessing
hardware
devices and so on) has to be written in something else. In CRuby these
libraries are written in C. In IronRuby these are written in a .NET
language (i.e. C#).

That being said, since IronRuby provides .NET interop then it is
conceivable
to write a Ruby library in Ruby and bounce out to the .NET framework
classes
as necessary. Obviously performance issues must be taken into account.
Also, there are issues discussed recently about packaging up such Ruby
libraries when deploying to Silverlight.

Pete

The .NET Socket library is a fairly thin layer that sits on top of
WinSock.
Clearly Silverlight would not be able to do this since WinSock is not a
standard API on other OSes. Also, Silverlight is going to have
additional
security restrictions that would prevent much of the Socket library from
work anyway.

To be honest, even the full .NET Framework socket implementation does
not
fully support all the features required by the Ruby socket library. I
have
been struggling to get the Socket class working - it is not pretty.

This is partly to do with the way the Ruby socket library is
implemented.
In particular, the Socket class provides access to low level C API
methods,
which differ from system to system. In the best case this results in
slightly inconsistent behaviour and different errors being raised. In
the
worst case it is not possible to write portable Ruby code that uses the
socket library, unless you restrict yourself to well defined methods.

It seems reasonable that, since the Ruby Socket library is somewhat OS
dependent anyway, we should agree upon a subset of the functionality to
support, particularly with regards to Silverlight. Perhaps this should
be
based upon common usage in programs like Mongrel? I suspect that like
many
of these things, we may end up having to code up some stuff in straight
.NET
(is this possible with socket-type things?)

Pete

John L. wrote:

Specifically on your contribution, I’m having a heck of a time getting it
to compile under Silverlight. I’m going to block off some more time
tomorrow
to see what I can do. Sockets are only partially implemented in
Silverlight, so I have to figure out the delta. Don’t worry about making
changes - once > I get it compiling under SL, I’ll check it in and you
can
modify my changes :slight_smile:

Thanks,
-John


Ironruby-core mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/ironruby-core

Peter Bacon D.:

pretty.
Do you think it would be worthwhile to just have folks use the .NET
Socket support in Silverlight and not bother having a “ruby” socket
implementation? In this case I still have to figure out how to
conditionally compile stuff against the same Initializer.Generated.cs -
I might have to add a pre-build step to force its generation.

Thanks,
-John

As an example, I tried this with the zlib library initially (well not
the
interop, just pure ruby). The result of which is Zliby (
http://zliby.rubyforge.org/), however the performance was so abysmial it
pretty much had to be rewritten in C# for it to be functional.

On Wed, May 7, 2008 at 10:33 AM, Peter Bacon D. <

/ideally/ we would be able to use RUBY_PLATFORM or some such to
determine if
we were on silverlight or not. We could have a socket.rb that loads the
functions differently depending on the platform (and throws not
implements
if it’s not applicable to Silverlight).

On Wed, May 7, 2008 at 10:38 AM, John L. (IRONRUBY)
rem[email protected]


Ironruby-core mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/ironruby-core


Ironruby-core mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/ironruby-core

This seems reasonable to me but then I am not a seasoned Ruby Sockets
user
so I don’t know what level of support developers would expect.

Certainly for a first release I don’t see why we couldn’t get away
without
Ruby Sockets support on Silverlight. I doubt anyone coding up a
Silverlight
Ruby app is going to worry that their code is portable to non-IronRuby
platform.

The big question is whether there are any libraries that require the
Ruby
Socket library that might be of use to people writing Silverlight apps.

Pete

C.J. Adams-Collier:

Sounds reasonable. Any thoughts on using the 'cil
http://gcc.gnu.org/projects/cli.html ’ back-end for gcc to emit IL
placeholders until there are enough free tuits to implement in C#?

I doubt that would generate verifiable CIL, so it’s a non-starter for
most of the things that we’re (especially Silverlight).

Thanks,
-John

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs