486199 - Support HTTP over SCTP

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

This bug is specifically for adding SCTP support to our HTTP networking code.  This will layered on top of NSPR supporting SCTP, which is bug 235681.

It sounds like a good first step is to start talking about what kinds of changes we might need architecturally to make the most out of the SCTP API, and any lessons learned from the initial version.

The rest of this comment are some notes from Preethi Natarajan at Cisco:

The current HTTP over SCTP design
(http://tools.ietf.org/html/draft-natarajan-http-over-sctp-01) exploits
SCTP streams to avoid head-of-line blocking. SCTP also offers other
services unavailable in TCP such as unordered delivery, partially
reliable transmission, message based transfer, multihoming support, more
secure connection establishment with a cookie mechanism and so on. 

We implemented HTTP over SCTP in the Apache server and Firefox browser.
Currenlty, Jon Leighton from Univ. of Delaware (CC'ed in this email) is
adapting NSPR to become SCTP-capable over a variety of platforms
including *BSD, Linux, Solaris, and Windows (Bugzilla thread:
https://bugzilla.mozilla.org/show_bug.cgi?id=383180). He has been
collaborating with Wan-Teh Chang on the same. 

Since SCTP streams work like multiple "parallel" streams within a
socket, it would've been ideal to have nsSocketTransport2 point to
multiple nsSocketIn(Out)putStreams, one for each SCTP In(Out)put stream.
However, nsSocketTransport2 can point to at most 1 In(Out)putStream and
the callback from nsHttpConnection and above is tightly coupled with
this In(Out)putStream. So I had to work around this at nsHttpConnection.
First, we'd like HTTP developers to vet this design/implementation and
suggest improvements if any.

There is an ongoing discussion at the HTTP WG about pipelining in HTTP
1.1 and how it relates to TCP vs. SCTP
(http://lists.w3.org/Archives/Public/ietf-http-wg/2009JanMar/0285.html).
We'd like to discuss how this affects the implementation of
nsHttpPipeline over SCTP (and TCP).

Comment 2

[Jason Duell]

Preethi,

We don't have a single networking "expert" in house at the moment.  I'm CC-ing the "usual suspects":  hopefully between us we can help you with the design issues involved with changing the necko (Mozilla networking layer == "necko") HTTP code to run over SCTP.

It sounds like you're mainly wondering if nsSocketTransport2 should be changed to accomodate multiple (In|Out)putStreams, or whether your existing workaround in nsHttpConnection is the right way to proceed.  Is that right?  Anything else?

Comment 3

[Preethi]

(In reply to comment #2)

Yes. At the moment Jon and I are interested in knowing if the changes to nsHttpConnection are "acceptable".

Comment 4

[Jason Duell]

Preeti/Jon,

Do you have a patch you can attach to this bug, so we can see exactly what changes you made to nsHttpConnection?  Or at least a more concrete description?

Comment 5

[Preethi]

Please find some documentation on the mods at: http://www.cis.udel.edu/~nataraja/ModsToHttpModule.pdf

I will also work with Jon to get the latest patch that uses SCTP-related NSPR APIs.

Comment 6

[Preethi]

Attachment: Patch of netwerk/base/src

Comment 7

[Preethi]

Attachment: Patch of netwerk/protocol/http/src

Comment 8

[Preethi]

Folks,

Attached are patches for two directories -- (i) netwerk/base/src and (ii) netwerk/protocol/http/src 

Mods in (i) are mainly in nsSocketTransport2.cpp. Added new methods to maintain the table (T) of {sctp stream id, queue of transactions} described in the attached doc.

Mods in (ii):

nsHttpConnection.cpp:

nsHttpConnection::OnReadSegment - After writing on nsSocketTransport, find out the SCTP stream id on which the HTTP request was written, and add the corresponding nsHttpTransaction to the head of the stream's queue in table T. 
nsHttpConnection::OnSocketReadable - first find out the SCTP stream (s) on which a response has been received. Find the head of s' queue in Table T and call Writesegments on this head transaction. If the complete response has been read, delete transaction from the head of queue.

Comment 9

[Preethi]

Hello,

If the description/mods appear complicated, I can try to break it down a little bit, or I can post patches/mods for each file? Let me know how I can help. Thanks.

Comment 10

[Preethi]

I am pleased to inform you that the NSPR community is reviewing Jon Leighton's patch for SCTP support  (https://bugzilla.mozilla.org/show_bug.cgi?id=383180). 

Jon is currently working on integrating the SCTP-capable NSPR into the latest Firefox source and he is hoping that someone from the Firefox developer community can provide him with pointers. Since Jon will do the integration, we expect that this effort would require minimal support from the Firefox maintainers. It'd be great if someone can provide Jon with the help that he needs.

Comment 11

[Michal Novotny [:michal]]

Preethi, I'm looking at your patches and they seem to be incomplete. I'm missing changes in nsIOutputStream.idl (WriteSegments2 method). Also could you please produce your patches using diff parameters -p and -U8 for better readability? Thanks.

Comment 12

[Jonathan Leighton]

Attachment: Partial SCTP patch for Firefox 3.0.11

Michal - I spoke to Preethi and we thought it would be better if I sent you the diff files since I'm working on a much more up-to-date version of firefox.  I've attached a diff that should show you all the relevant modifications for SCTP, based on firefox 3.0.11.  I excluded mozilla/nsprpub since the changes there are extensive and already being reviewed under another bug report, and I excluded mozilla/configure.  I didn't dig out the other configure scripts.  Configure related stuff is more of a work-in-progress and should be ignored for now.  In fact, you can safely ignore the few things in the diff file that aren't in mozilla/modules, mozilla/netwerk, or mozilla/xpcom.  I can post diffs of those three directories if you'd rather work with that.

Comment 13

[Michal Novotny [:michal]]

Jonathan, I was able to compile SCTP support with firefox 3.0.15pre after some tweaking. Normal (TCP) HTTP is broken and I can't find any web server that is using SCTP. Although www.sctp.org is listening on SCTP port 80 it doesn't return any data (or something is broken in the firefox). I would like to test it locally. Is there available any uptodate patch against apache or any other httpd server? I can't compile the one at http://www.sctp.org/download.html.

Comment 14

[Jonathan Leighton]

It's great to hear that you were able to compile with SCTP support, though I'm curious about how you did that since I didn't post the patch for mozilla/nsprpub.  Here are a couple of options for testing it out.  I have a version of Apache's httpd running over SCTP at www.buzet.pc.cis.udel.edu:4000 which you should be able to access.  The PEL logo at the top of that page is an active link, but the links in the page it brings up are almost all inaccessible with SCTP Firefox.  You can also get a tarball of the SCTP version of httpd from www.cis.udel.edu/~leighton/httpd-apr-2007.tar.  Updates for the SCTP version of httpd are in progress, but this version is known to work on FreeBSD (i386) 6.1, 7.1 and 7.2.  FreeBSD (i386) 7.0 is probably a good bet, too.

Comment 15

[Michal Novotny [:michal]]

(In reply to comment #14)
> It's great to hear that you were able to compile with SCTP support, though I'm
> curious about how you did that since I didn't post the patch for
> mozilla/nsprpub.  Here are a couple of options for testing it out.  I have a

Of course I've applied the patch from the bug #383180 too.

> version of Apache's httpd running over SCTP at www.buzet.pc.cis.udel.edu:4000
> which you should be able to access.  The PEL logo at the top of that page is an
> active link, but the links in the page it brings up are almost all inaccessible
> with SCTP Firefox.  You can also get a tarball of the SCTP version of httpd
> from www.cis.udel.edu/~leighton/httpd-apr-2007.tar.  Updates for the SCTP

I can't resolve host www.buzet.pc.cis.udel.edu but in the end I was able to compile and run the server locally.

Comment 16

[Michal Novotny [:michal]]

I've looked at the code and I've also tried to compile and run it on linux. The good news is that with some small changes it works on linux too.

Modifications outlined in ModsToHttpModule.pdf are in my opinion reasonable. But the problem I see is that the stream transaction queue is in fact maintained by nsSocketTransport and not by nsHttpConnection. I think that it is OK if nsSocketTransport provides {This,Next}Sctp{In,Out}putStreamId values but the table should be maintained by nsHttpConnection or maybe by nsHttpConnectionMgr.

Also please note that the WriteSegments2() method is unacceptable. Btw. the interface nsIOutputStream is frozen. I'm not sure I understand exactly why it is a problem to call nsReadSegmentFun in a loop. Anyway if you know how much data it is safe to read then you can pass this amount to WriteSegment(). Alternatively you can break the loop by throwing an error in nsHttpTransaction::WritePipeSegment().

I think that there is a bug in the nspr patch. In pt_Sctp_Recvmsg() you're passing msg_flags instead of osflags as an parameter to _PR_MD_SCTP_RECVMSG(), but it works since PR_MSG_PEEK and MSG_PEEK are the same. I've found it while I was investigating why the data is consumed although PR_MSG_PEEK was specified. It is good to know that MSG_PEEK doesn't work on linux with lksctp-tools-1.0.9 and older. I had to build and install lksctp-tools-1.0.10 on my own since 1.0.9 is still the newest version for Fedora 10.

Comment 17

[Jonathan Leighton]

(In reply to comment #15)
> (In reply to comment #14)
> Of course I've applied the patch from the bug #383180 too.

The NSPR patch is based on a more recent version of NSPR than that used by firefox 3.0.x, but the differences aren't relevant to SCTP and I'm not surprised that that worked fine.

> I can't resolve host www.buzet.pc.cis.udel.edu but in the end I was able to
> compile and run the server locally.

My mistake - I shouldn't have included "www."  Point you browser to buzet.pc.cis.udel:4000 instead.

Comment 18

[Preethi]

(In reply to comment #16)
> I've looked at the code and I've also tried to compile and run it on linux. The
> good news is that with some small changes it works on linux too.
> 
> Modifications outlined in ModsToHttpModule.pdf are in my opinion reasonable.
> But the problem I see is that the stream transaction queue is in fact
> maintained by nsSocketTransport and not by nsHttpConnection. I think that it is
> OK if nsSocketTransport provides {This,Next}Sctp{In,Out}putStreamId values but
> the table should be maintained by nsHttpConnection or maybe by
> nsHttpConnectionMgr.

At this point, it seems like this should be possible. We'll go ahead and try to implement this as follows:
- Keep {This,Next}Sctp{In,Out}putStreamId attributes methods in nsISocketTransport.idl, and the corresponding Get methods in nsSocketTransport2.cpp
- Remove stream transaction queue related methods (Gethead, Deletehead
etc.) from nsISocketTransport.idl
- Move the queue methods into nsHttpConnection.h and nsHttpConnection.cpp. These methods can be private methods.
- Use nsHttpConnection's mSocketTransport object to get the Sctp Stream Ids, and use the stream id to manage the transaction queue

> 
> Also please note that the WriteSegments2() method is unacceptable. Btw. the
> interface nsIOutputStream is frozen. I'm not sure I understand exactly why it
> is a problem to call nsReadSegmentFun in a loop. Anyway if you know how much
> data it is safe to read then you can pass this amount to WriteSegment().
> Alternatively you can break the loop by throwing an error in
> nsHttpTransaction::WritePipeSegment().

Note that pieces of different responses can be received interleaved at the SCTP layer. I.e., when the app does 2 back-to-back read()s on the SCTP socket, read 1 can return a piece of transaction 1 from stream 1 and read 2 can return a piece of transaction 2 from stream 2. So, the app should not read in a loop, since the subsequent read() could be for another transaction. WriteSegments2() is the non-loop version of WriteSegments(). WriteSegments2() has a valid implementation in just nsPipe3.cpp, the other objects have a null implementation.

The first option you specify -- pass the amount of data to read to WriteSegment(). This would be possible if somehow the app "knows" the number of bytes that will be read in the next read() on the socket. Is it possible to know that information?

The second option -- throwing an error from nsHttpTransaction::WritePipeSegment(). Can you elaborate more on this (my memory has become a little rusty on the code details..)? Basically, WritePipeSegment should either (i) return after just 1 read() or (ii) stop reading if it knows that the _next_ read() is not for _this_ transaction.

Maybe there are other options?

> 
> I think that there is a bug in the nspr patch. In pt_Sctp_Recvmsg() you're
> passing msg_flags instead of osflags as an parameter to _PR_MD_SCTP_RECVMSG(),
> but it works since PR_MSG_PEEK and MSG_PEEK are the same. I've found it while I
> was investigating why the data is consumed although PR_MSG_PEEK was specified.

Thanks for picking this up.  I've fixed it in my source, and if someone starts reviewing my patch again I'll post an update.  Right now the review seems to be in limbo.

> It is good to know that MSG_PEEK doesn't work on linux with lksctp-tools-1.0.9
> and older. I had to build and install lksctp-tools-1.0.10 on my own since 1.0.9
> is still the newest version for Fedora 10.

That's very good to know, though there's a simple workaround for our purposes.  When we peek we aren't really interested in looking at any bytes of data; we just want the sctp_sndrcvinfo structure filled in.  On FreeBSD, calling sctp_recvmsg with MSG_PEEK doesn't fill in the sctp_sndrcvinfo structure unless you peek at at least one byte, so the code I sent you does that.  The linux lksctp implementation doesn't have this problem so you can just peek with a byte count of zero to sidestep that problem.

Comment 19

[Michal Novotny [:michal]]

(In reply to comment #18)
> The second option -- throwing an error from
> nsHttpTransaction::WritePipeSegment(). Can you elaborate more on this (my
> memory has become a little rusty on the code details..)?

Throwing error in nsHttpTransaction::WritePipeSegment would break the loop at http://hg.mozilla.org/mozilla-central/annotate/001a1805c476/xpcom/io/nsPipe3.cpp#l1140 (i.e. WriteSegments would call the reader only once just like in case of WriteSegments2).


+           /* The following call causes mSocketIn's Read() to do a MSG_PEEK
+            * Look at OnWriteSegment and mSocketIn's Read() for comments.
+            */
+
+           PRUint32 temp;
+           rv = OnWriteSegment(NULL, 1, &temp);

This code is a little tricky. It would be IMO better to move the MSG_PEEK into nsSocketTransport::GetNextSctpInputStreamId. Actually the code is there but it is commented out. Why is that?


> Basically, WritePipeSegment should either (i) return after just 1 read()
> or (ii) stop reading if it knows that the _next_ read() is not for _this_
> transaction.

NextSctpInputStreamId is valid after calling PR_SctpRecvmsg only when SCTP_HAVE_EXTRCVINFO is defined, right? If the MSG_PEEK was done in nsSocketTransport::GetNextSctpInputStreamId then you would always have correct NextSctpInputStreamId in nsHttpConnection::OnWriteSegment and you could return some specific success code (something like NS_SCTP_DO_NOT_CALL_AGAIN) when ThisSctpInputStreamId != NextSctpInputStreamId. nsHttpTransaction::WritePipeSegment would throw some error to break the loop when it gets NS_SCTP_DO_NOT_CALL_AGAIN. I hope it is understandable what I wrote here.

Comment 20

[Jonathan Leighton]

Attachment: SCTP patch for Firefox 3.0.11, excluding nsprpub

This patch adds SCTP support to Firefox 3.0.11.  It does not include modifications to nsprpub, which should be made by applying the SCTP patch for NSRP 4.8.1 (see bug 383180).  The top level configure script is not included, and must be recreated after patching.  The configure script must be run using the --enable-sctp option.

Comment 21

[Jonathan Leighton]

(In reply to comment #19)
> This code is a little tricky. It would be IMO better to move the MSG_PEEK into
> nsSocketTransport::GetNextSctpInputStreamId.

I agree, and that's what I'm doing now.


> NextSctpInputStreamId is valid after calling PR_SctpRecvmsg only when
> SCTP_HAVE_EXTRCVINFO is defined, right? If the MSG_PEEK was done in
> nsSocketTransport::GetNextSctpInputStreamId then you would always have correct
> NextSctpInputStreamId in nsHttpConnection::OnWriteSegment and you could return
> some specific success code (something like NS_SCTP_DO_NOT_CALL_AGAIN) when
> ThisSctpInputStreamId != NextSctpInputStreamId.
> nsHttpTransaction::WritePipeSegment would throw some error to break the loop
> when it gets NS_SCTP_DO_NOT_CALL_AGAIN. I hope it is understandable what I
> wrote here.

Yes – very understandable.  nsHttpConnection::OnWriteSegment now tries to update NextSctpInputStreamId after calling nsSocketInputStream::Read, and if appropriate, returns NS_SCTP_STREAM_ID_CHANGED.  Since new data has actually been read in this case, nsHttpTransaction::WritePipeSegment passes the error on to nsPipeOutputStream::WriteSegments, which adjusts the counts appropriately before checking for NS_SCTP_STREAM_ID_CHANGED and breaking the loop.


Other Changes:

The StreamTransactionTable and associated methods have been moved from nsSocketTransport to nsHttpConnection.

SCTP stream related variables in the NSPR file descriptor structure are no longer needed and have been removed.  I'll be updating the NSPR patch to remove these from the file descriptor structure.  (I've wanted to get rid of them.)

nsPipeOutputStream::WriteSegments2 has been removed.

The preferences in all.js and firefox.js have been adjusted for SCTP, and default to pipelining.

The --enable-sctp option should now work correctly with the top level configure script.  The code is still broken if you exclude this option.

The code should work “as is” on linux, even with the broken MSG_PEEK.

Comment 22

[Michal Novotny [:michal]]

This comment is just about WriteSegments2/NS_SCTP_STREAM_ID_CHANGED change. More comments will follow later.

NS_SCTP_STREAM_ID_CHANGED can't be defined in xpcom since it is network related. It should be defined in netwerk/base/public/nsNetError.h and it should be a success code and not an error (see below).

In fact there should be no change in xpcom tree. Why is this change? :

-#define DEFAULT_SEGMENT_SIZE  4096 
+#define DEFAULT_SEGMENT_SIZE  4224      // JTL - 11/8/07, per PN 


You're right that you can't throw an error on SCTP id change in nsHttpTransaction::WritePipeSegment immediately after calling nsHttpConnection::OnWriteSegment because nsPipeOutputStream::mLogicalOffset needs to be updated. It can be done as follows: 

The call stack is:
  nsHttpConnection::OnWriteSegment
  nsHttpTransaction::WritePipeSegment
  nsPipeOutputStream::WriteSegments
  nsHttpTransaction::WriteSegments

In nsHttpTransaction::WriteSegments set a new member variable mSctpStreamIdChanged to PR_FALSE before calling mPipeOut->WriteSegments.

In nsHttpTransaction::WritePipeSegment throw some error before calling trans->mWriter->OnWriteSegment when mSctpStreamIdChanged is PR_TRUE. And set the mSctpStreamIdChanged to PR_TRUE when trans->mWriter->OnWriteSegment return NS_SCTP_STREAM_ID_CHANGED. This ensures that nsPipeOutputStream::mLogicalOffset will be always updated correctly without any change in xpcom/nsPipe3.cpp.

nsHttpConnection::OnWriteSegment should return NS_SCTP_STREAM_ID_CHANGED which is a SUCCESS return value not an error, so you don't have to do things like "if (NS_SCTP_STREAM_ID_CHANGED == rv || NS_SUCCEEDED(rv))".

Comment 23

[Michal Novotny [:michal]]

PR_SctpRecvmsg is called also for non-sctp socket types. The current code crashes when you try to visit https page. Normal TCP socket is created in nsSSLSocketProvider. See backtraces below.

Crash:
#0  0x002c1430 in __kernel_vsyscall ()
#1  0x049d9460 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0x049dae28 in abort () at abort.c:88
#3  0x00d916b2 in PR_Assert (s=0xdba635 "!\"I/O method is invalid\"", 
    file=0xdba5fc "/opt/moz/TRUNK_new3/mozilla/nsprpub/pr/src/io/priometh.c", ln=86)
    at /opt/moz/TRUNK_new3/mozilla/nsprpub/pr/src/io/prlog.c:550
#4  0x00d8d162 in _PR_InvalidInt () at /opt/moz/TRUNK_new3/mozilla/nsprpub/pr/src/io/priometh.c:86
#5  0x00d8fe13 in pl_DefSctp_recvmsg (fd=0xef155e80, buf=0xf72fee7b, amount=0, from=0xf72fede4, 
    fromlen=0xf72fee74, sinfo=0xf72fee50, msgflags=0xf72fee70, timeout=0)
    at /opt/moz/TRUNK_new3/mozilla/nsprpub/pr/src/io/prlayer.c:406
#6  0x00d8fe13 in pl_DefSctp_recvmsg (fd=0xef155eb0, buf=0xf72fee7b, amount=0, from=0xf72fede4, 
    fromlen=0xf72fee74, sinfo=0xf72fee50, msgflags=0xf72fee70, timeout=0)
    at /opt/moz/TRUNK_new3/mozilla/nsprpub/pr/src/io/prlayer.c:406
#7  0x00d8fe13 in pl_DefSctp_recvmsg (fd=0xf4fcf520, buf=0xf72fee7b, amount=0, from=0xf72fede4, 
    fromlen=0xf72fee74, sinfo=0xf72fee50, msgflags=0xf72fee70, timeout=0)
    at /opt/moz/TRUNK_new3/mozilla/nsprpub/pr/src/io/prlayer.c:406
#8  0x00d8d574 in PR_SctpRecvmsg (fd=0xf4fcf520, msg=0xf72fee7b, msgsz=0, from=0xf72fede4, fromlen=0xf72fee74, 
    sinfo=0xf72fee50, msg_flags=0xf72fee70, timeout=0)
    at /opt/moz/TRUNK_new3/mozilla/nsprpub/pr/src/io/priometh.c:227
#9  0x010db83a in nsSocketTransport::UpdateNextSctpInputStreamId (this=0xef12a530)
    at /opt/moz/TRUNK_new3/mozilla/netwerk/base/src/nsSocketTransport2.cpp:1517
#10 0x010dbcd4 in nsSocketTransport::GetNextSctpInputStreamId (this=0xef12a530, SCTPStreamID=0xf29ae690)
    at /opt/moz/TRUNK_new3/mozilla/netwerk/base/src/nsSocketTransport2.cpp:1547
#11 0x01162cf0 in nsHttpConnection::OnInputStreamReady (this=0xf29ae630, in=0xef12a610)
    at /opt/moz/TRUNK_new3/mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp:876


Socket creation:
#0  nsSSLIOLayerNewSocket (family=2, host=0xf11fb968 "localhost", port=443, proxyHost=0x0, proxyPort=0, 
    fd=0xf72ff05c, info=0xf72fece4, forSTARTTLS=0)
    at /opt/moz/TRUNK_new3/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:1976
#1  0x01d4fff5 in nsSSLSocketProvider::NewSocket (this=0xf2c2cc00, family=2, host=0xf11fb968 "localhost", 
    port=443, proxyHost=0x0, proxyPort=0, flags=0, _result=0xf72ff05c, securityInfo=0xf72fece4)
    at /opt/moz/TRUNK_new3/mozilla/security/manager/ssl/src/nsSSLSocketProvider.cpp:71
#2  0x011f9458 in nsSocketTransport::BuildSocket (this=0xf11b72d0, fd=@0xf72ff05c, proxyTransparent=@0xf72ff058, 
    usingSSL=@0xf72ff054) at /opt/moz/TRUNK_new3/mozilla/netwerk/base/src/nsSocketTransport2.cpp:1095

Comment 24

[Michal Novotny [:michal]]

Jonathan,

I think it would be better if you were working with mozilla-central trunk. SCTP support is a big change and AFAIK there is no chance to get it into branches 1.9, 1.9.1 or 1.9.2. The patches you are providing are reversed. Next time please swap directories when running diff, or preferably make the patch using "hg diff" command.

The patch needs a lot of changes in coding style. I'll point out every type of change only once (e.g. name of member variables, comments, ...) but please change accordingly all your code. See https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide for common rules.

All changes in browser/app/profile/firefox.js and modules/libpref/src/init/all.js that are not really needed should be removed.

Remove all comments " // JTL - XX/XX/XX"

Remove all code that is commented out. It makes reading the patches easier.

> +    // Get NextSctpInputStreamId; 
> +    readonly attribute PRInt32 nextSctpInputStreamId; 
> + 
> +    // Get ThisSctpInputStreamId; 
> +    //readonly attribute PRInt32 thisSctpInputStreamId; 
...

Move the description of all new attributes from nsSocketTransport2.cpp to nsISocketTransport.idl.

> +    else 
> +        n = PR_Read(fd, buf, count); 
> 
>      LOG(("  PR_Read returned [n=%d]\n", n));

Wrong message is logged in case of SCTP.

> +    else 
> +        n = PR_Write(fd, buf, count); 
>  
>      LOG(("  PR_Write returned [n=%d]\n", n)); 

The same as above.

>      , mOutput(this) 
> +    // JTL - 7/5/07, Initialize SCTP stream info. 
> +    , NextSctpInputStreamId (-1) 
> +    , NextSctpOutputStreamId (-1) 
> +    , NumberOfSctpStreams (0) 
>  { 

Name of all member variables should begin with "m".


> +            // Subscribe to sctp_data_io_event. 
> +            PRSocketOptionData sod; 
> +            memset(&sod, 0, sizeof(sod)); 
> +            sod.option = PR_SockOpt_Sctp_Events; 
> +            sod.value.events.sctp_data_io_event = 1; 
> +            status = PR_SetSocketOption(fd, &sod); 
> +            NS_ASSERTION(status == PR_SUCCESS, "failed to subscribe to sctp_events"); 

You're subscribing to the sctp events but I don't see the code that handles the events. Am I missing something? Also are all NS_ASSERTION in nsSocketTransport::BuildSocket really assertions? Shouldn't they be handled as errors?

> +            sod.value.initmsg.sinit_num_ostreams = 10; 

Use either a define or make it a preference value if users are supposed to tweak it.

> +        // JTL - 12/20/07 
> +        // Need to get max number of outgoing SCTP streams. 
> +        if (PR_GetDescType(fd) == PR_DESC_SOCKET_SCTP_1TO1) { 
> +            PRSocketOptionData sod; 
> +            memset(&sod, 0, sizeof(sod)); 
> +            sod.option = PR_SockOpt_Sctp_Status; 
> +            status = PR_GetSocketOption(fd, &sod); 
> +            NS_ASSERTION(status == PR_SUCCESS, "failed to get max number of outgoing SCTP streams"); 
> +            NumberOfSctpStreams = sod.value.status.sstat_outstrms; 
> +        } 

The same code is used several times. Make this a function to avoid duplication. And again, IMHO if PR_GetSocketOption() fails it is an error and not just assertion. You should handle it and return some error to the caller.

> +    if (n >= 0) { 
> +        if (0 == n && PR_NOT_CONNECTED_ERROR == PR_GetError()) 
> +            rv = NS_BASE_STREAM_CLOSED; 

Order in the if statement should be the same for whole source file. I.e. common style for nsSocketTransport2.cpp is:
if (n == 0 && PR_GetError() == PR_NOT_CONNECTED_ERROR)

> +// Get the SCTP stream on which THIS piece of data was written 
> +NS_IMETHODIMP 
> +nsSocketTransport::GetNumberOfSctpStreams(PRInt32* NumberOfStreams) 

The description is wrong.

> +                rv = UpdateNextSctpInputStreamId(); 
> +                // Handle errors 
> +                if (NS_FAILED(rv)) 
> +                    mInput.OnSocketReady(NS_ERROR_FAILURE); 
> +                else 
> +                    mInput.OnSocketReady(NS_OK);

Is there any reason why not pass to the error code? I.e. just mInput.OnSocketReady(UpdateNextSctpInputStreamId())

> -#define NS_HTTP_MAX_PIPELINED_REQUESTS 8. 
> +#define NS_HTTP_MAX_PIPELINED_REQUESTS 1000   // JTL 11/9/07, per PN 

Why 1000?

> +        delete (nsVoidArray *)StreamTransactionTable.ElementAt(i);

If you switch to mozilla-central tree use static_cast<nsVoidArray*>

> +    PRInt32 SctpOutputStream;

Name of local variable should begin with small letter -> sctpOutputStream.

> +            if (rv == NS_BASE_STREAM_WOULD_BLOCK){ 

Nit: add a space before the bracket

> +    nsAHttpTransaction *mThisTransaction;

Not a member variable, rename to thisTransaction.

> +nsHttpConnection::AddTransaction(PRInt32 SCTPStreamID, nsAHttpTransaction* trans)

Arguments should begin with "a" -> aSCTPStreamID, aTrans

> +    NS_ASSERTION(trans != NULL, "NULL TRANSACTION");

Use NS_ENSURE_ARG_POINTER

> +        transArray = new nsVoidArray();

Add OOM check.

> +    PRBool status =  transArray->AppendElement(mTrans);

Don't declare the mTrans variable and use aTrans directly.

> +    printf("nsHttpTransaction::WriteSegments: *** >>> SHOULD NOT BE HERE <<< ***\n");     // JTL

Change all printf to either NS_ERROR or NS_WARNING.

Comment 25

[Jonathan Leighton]

(In reply to comment #22)
> NS_SCTP_STREAM_ID_CHANGED can't be defined in xpcom since it is network
> related. It should be defined in netwerk/base/public/nsNetError.h and it should
> be a success code and not an error (see below).

Done.  In nsNetError.h I've defined NS_SCTP_INPUT_STREAM_ID_CHANGED as a success code and NS_SCTP_WRONG_TRANSACTION as a failure code.

> In fact there should be no change in xpcom tree. Why is this change? :
> 
> -#define DEFAULT_SEGMENT_SIZE  4096 
> +#define DEFAULT_SEGMENT_SIZE  4224      // JTL - 11/8/07, per PN 

This is left over from earlier work and is no longer needed.  It has been removed.  There are now no modifications remaining in the xpcom tree.

> In nsHttpTransaction::WriteSegments set a new member variable
> mSctpStreamIdChanged to PR_FALSE before calling mPipeOut->WriteSegments.
> 
> In nsHttpTransaction::WritePipeSegment throw some error before calling
> trans->mWriter->OnWriteSegment when mSctpStreamIdChanged is PR_TRUE. And set
> the mSctpStreamIdChanged to PR_TRUE when trans->mWriter->OnWriteSegment return
> NS_SCTP_STREAM_ID_CHANGED. This ensures that nsPipeOutputStream::mLogicalOffset
> will be always updated correctly without any change in xpcom/nsPipe3.cpp.
> 
> nsHttpConnection::OnWriteSegment should return NS_SCTP_STREAM_ID_CHANGED which
> is a SUCCESS return value not an error, so you don't have to do things like "if
> (NS_SCTP_STREAM_ID_CHANGED == rv || NS_SUCCEEDED(rv))".

Done.  The new member variable nsHttpTransaction::mSctpInputStreamIdChanged has been added.  When appropriate, nsHttpConnection::OnWriteSegment now returns the success code NS_SCTP_INPUT_STREAM_ID_CHANGED, and nsHttpTransaction::WritePipeSegment now returns the failure code NS_SCTP_WRONG_TRANSACTION.

Comment 26

[Jonathan Leighton]

Attachment: SCTP patch for Firefox 3.0.11, excluding nsprpub

Sorry for the long delay in responding Michal.  I've modified the code so that, when built with --enable-sctp, sockets are not assumed to be SCTP sockets.  Typically, PRDescType is now checked where appropriate (e.g. before deciding to call PR_SctpRecvmsg or PR_Read).  The code works with (my limited testing of) https pages, however page loading is very slow and I haven't been able to determine why.  I'd appreciate any help with this as I'm unfamiliar with https/SSL.  Other changes include fixing the code to properly build Firefox when --enable-sctp is _not_ specified, and greatly simplifying the modifications to nsHttpPipeline::ReadSegments and nsHttpPipeline::FillSendBuf.  These modifications require nsHttpPipeline::FillSendBuf to be able to determine if nsHttpConnection is using an SCTP socket, and comments on the suitability of my approach would be welcome.

I'll start working on converting the code to be based on the central trunk, and cleaning up the code to comply with the expected coding practices per comment 24 above.

- Jon

Comment 27

[Michal Novotny [:michal]]

Jon, I'll take a look at the https performance. Please add parameters -U8 and -p to the diff and post the patch again. Thanks.

Comment 28

[Jonathan Leighton]

Attachment: SCTP patch for Firefox 3.0.11, excluding nsprpub

Apologies Michal - here's the corrected patch.

Comment 29

[Jonathan Leighton]

Attachment: SCTP patch based on mozilla-central, including NSPR changes

This patch is based on mozilla-central rev 58b03899d500 and includes the SCTP related changes to NSPR.  If you build on FreeBSD (as I do), you will also need to apply the proposed patches for bugs 535942 and 544590.

Comment 30

[Jonathan Leighton]

I'm now working with the mozilla-central trunk, and (hopefully) the code now complies with the coding style guidelines.

> All changes in browser/app/profile/firefox.js and
> modules/libpref/src/init/all.js that are not really needed should be removed.
The only changes now remaining are those for homepage_override.mstone, blocklist, safebrowsing and safebrowsing.malware, because enabling these preferences creates connections to sites that do not support SCTP, and those related to pipelining, because the primary benefit of HTTP over SCTP comes from highly pipelined transfers.

> Remove all comments " // JTL - XX/XX/XX"
Done.

> Remove all code that is commented out. It makes reading the patches easier.
Done.

> Move the description of all new attributes from nsSocketTransport2.cpp to
> nsISocketTransport.idl.
Done.

> Wrong message is logged in case of SCTP.
Fixed.

> Name of all member variables should begin with "m".
Fixed.

> You're subscribing to the sctp events but I don't see the code that handles the
> events. Am I missing something? Also are all NS_ASSERTION in
> nsSocketTransport::BuildSocket really assertions? Shouldn't they be handled as
> errors?
The only event that I'm subscribing to is the data_io_event.  Subscription to this event means that the local SCTP stack will fill in the PRSctp_SendRcvInfo (or PRSctp_ExtRcvInfo) structure before returning from the PR_SctpRecvmsg call.  When I examine this data structure to learn the SCTP stream id number of a message, I'm "handling" the event.  The assertions are now handled as errors.

> > +            sod.value.initmsg.sinit_num_ostreams = 10;
> Use either a define or make it a preference value if users are supposed to
> tweak it.
This is now set with the network.http.sctp.max-number-of-outgoing-streams user preference.

> The same code is used several times. Make this a function to avoid duplication.
> And again, IMHO if PR_GetSocketOption() fails it is an error and not just
> assertion. You should handle it and return some error to the caller.
Done.

> Order in the if statement should be the same for whole source file. I.e. common
> style for nsSocketTransport2.cpp is:
> if (n == 0 && PR_GetError() == PR_NOT_CONNECTED_ERROR)
Fixed here and elsewhere.

> > +// Get the SCTP stream on which THIS piece of data was written
> > +NS_IMETHODIMP
> > +nsSocketTransport::GetNumberOfSctpStreams(PRInt32* NumberOfStreams)
> The description is wrong.
Fixed.

> Is there any reason why not pass to the error code? I.e. just
> mInput.OnSocketReady(UpdateNextSctpInputStreamId())
Changed.

> > -#define NS_HTTP_MAX_PIPELINED_REQUESTS 8.
> > +#define NS_HTTP_MAX_PIPELINED_REQUESTS 1000   // JTL 11/9/07, per PN
> Why 1000?
Good question.  The idea is to ensure that any reasonable web page can avoid head-of-line blocking between different objects by simultaneously loading each object on an independent stream.  I seem to recall reading that 40-50 objects is an typical range for many of the most popular websites, so I've reset this value to 200 in an effort to cover most cases.  More information on the number of objects per web page, and more experience using HTTP over SCTP, would provide better guidance on where to set this value for SCTP.

> If you switch to mozilla-central tree use static_cast<nsVoidArray*>
Changed here and elsewhere.

> Name of local variable should begin with small letter -> sctpOutputStream.
Fixed here and elsewhere.

> > +            if (rv == NS_BASE_STREAM_WOULD_BLOCK){
> Nit: add a space before the bracket
Fixed.

> > +    nsAHttpTransaction *mThisTransaction;
> Not a member variable, rename to thisTransaction.
Done.

> Arguments should begin with "a" -> aSCTPStreamID, aTrans
Fixed here and elsewhere.  This convention is frequently violated by existing code.  In cases where I've overloaded an existing function that does not follow this convention, I have chosen to remain consistent with the existing code.

> > +    NS_ASSERTION(trans != NULL, "NULL TRANSACTION");
> Use NS_ENSURE_ARG_POINTER
Done.

> > +        transArray = new nsVoidArray();
> Add OOM check.
Done.

> > +    PRBool status =  transArray->AppendElement(mTrans);
> Don't declare the mTrans variable and use aTrans directly.
Done.

> > +    printf("nsHttpTransaction::WriteSegments: *** >>> SHOULD NOT BE HERE <<< ***\n");     // JTL
> Change all printf to either NS_ERROR or NS_WARNING.
Done.

Comment 31

[Matthias-Christian Ott]

Any news on this?

Comment 32

[Jonathan Leighton]

I'm afraid not.  Michal Novotny hasn't had time to continue corresponding with me on this, and I've been busy with other activities.  Do you have a particular interest in this, or just want to know if it's still active?

Comment 33

[Mike]

Bug 855620 seems to be related to SCTP (already in Firefox?).

Comment 34

[Patrick McManus [:mcmanus]]

the web has brought sctp multistreaming to http in the form of h2.. rtc of course uses it for data channels as well. further benefits might come from a userspace transport (some downstream evolution of quic) - but we're unlikely to try and trail blaze sctp directly for http

Comment 35

[Elmar Stellnberger]

I regret that Firefox still has no proficient SCTP support! HTTP/2 over TCP does not solve the head-of-line blocking problem (but SCTP does). This is a major drawback. If one of many multiplexed HTTP/2 / SPDY streams gets blocked all other concurrent streams get blocked by TCP head-of-line blocking at the same time. And TCP needs to wait 4 RTTs until it acknowledges a packet as lost. An RTT can be up to 1 second in GPRS. SPDY over QUIC (sometimes already called HTTP/3) promises to solve this issue but I believe SPDY is a bad protocol. Apart from the fact that it is still not mature for use it suffers from a lower throughput and a higher overhead compared to TCP/SCTP. I would not rely on that as a new standard just because Google promotes it. Even HTTP/2 is a very complicated protocol compared to HTTP/1.1. However huffman header compression can not outweigh the additional overhead introduced by framing which is needed to multiplex several streams over one TCP connection. SCTP has some other interesting features like multihoming which can be used to avoid rebuffering of a video transmission if the IP address changes when switching from Wifi to 3G. Overall SCTP is a much simpler and more performant solution than SPDY/QUIC (as called HTTP/3 by Google).