40778 - Support for layered nonblocking connect

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P3)

Description

[Wan-Teh Chang]

NSPR doesn't support layered nonblocking
connect if the layer needs to do additional
socket I/O (such as talking to a SOCKS
server) after the network connection is
established.

The current procedure of doing nonblocking
connect in NSPR is the following three steps:
    PR_Connect
    PR_Poll
    PR_GetConnectStatus
(The reason we cannot retry PR_Connect
after PR_Poll is that it may raise
SIGPIPE on some System V Unix variants
if the nonblocking connect fails.) The
assumption is that the outcome of the
nonblocking connect should be known
when PR_Poll returns.  This assumption
is not true for layered nonblocking
connect that requires a sequence like
this:
    PR_Connect
    PR_Poll
    network connection is established
    do some more socket i/o
    PR_Poll
    do some more socket i/o
    .
    .
    .
    PR_Poll
    do some more socket i/o
    done

We need a new NSPR I/O method in which
a layer can do the additional socket i/o
during the nonblocking connect process.
Suppose this new function is called
PR_ConnectContinue.  Then the nonblocking
connect procedure would look like:
    PR_Connect
    do {
        PR_Poll
        rv = PR_ConnectContinue
    } while (rv == PR_IN_PROGRESS_ERROR)

(I was tempted to just convert the existing
PR_GetConnectStatus into an I/O method, but
this would break binary compatibility.  Sigh.)

Comment 1

[Wan-Teh Chang]

Does the name PR_ConnectContinue sound reasonable?
Any suggestion of a better name?

Here is my proposed specification of the function.
    PRStatus PR_ConnectContinue(
        PRFileDesc *fd,
        PRInt16 out_flags
    );

    Parameters
    - fd: the file descriptor
    - out_flags: the out_flags field of the poll
      descriptor returned by PR_Poll

    Return Values
    The function returns PR_SUCCESS if the nonblocking
    connect has successfully completed.  If the nonblocking
    connect is still in progress, the function returns
    PR_FAILURE with the error code PR_IN_PROGRESS_ERROR.
    In this case the caller should poll on the file
    descriptor for the in_flags PR_POLL_WRITE|PR_POLL_EXCEPT
    and retry PR_ConnectContinue when PR_Poll returns.
    If the nonblocking connect has failed, the function
    returns PR_FAILURE with an error code other than
    PR_IN_PROGRESS_ERROR.

A new I/O method connectcontinue would be added to
the PRIOMethods table.  PR_ConnectContinue would
simply invoke that method.
    PR_IMPLEMENT(PRStatus) PR_ConnectContinue(
        PRFileDesc *fd, PRInt16 out_flags)
    {
        return((fd->methods->connectcontinue)(fd,out_flags));
    }

Each I/O layer would define its own connectcontinue
method to perform any additional socket I/O required
by the protocol.  The connectcontinue method of the NSPR
layer would consist of pretty much the current code
of PR_GetConnectStatus.

The existing PR_GetConnectStatus function would be
deprecated (because it doesn't support layering well)
and defined in terms of the new PR_ConnectContinue
function.
    PRStatus PR_GetConnectStatus(PRPollDesc *pd)
    {
        /* Find the NSPR layer and invoke its connectcontinue method */
        PRFileDesc *bottom = PR_GetIdentitiesLayer(pd->fd, PR_NSPR_IO_LAYER);

        if (NULL == bottom) {
            PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
            return PR_FAILURE;
        }
        return bottom->methods->connectcontinue(pd->fd, pd->out_flags);
    }

Would this meet your requirements?

Comment 2

[ruslan]

Sounds cool to me. We need to make sure that the new nspr gets into mozilla 
builds once this is fixed

Comment 3

[Wan-Teh Chang]

Ruslan, you mean this needs to get into
Mozilla 1.0/Netscape 6?  Is this blocking
any dogfood or nsbeta2+ bugs?

My current plan is to get this into NSPR 4.1,
which is scheduled for 09/01 and won't be
used by Mozilla 1.0/Netscape 6.  If this
feature is required by Mozilla 1.0/Netscape 6,
I'll need to rename NSPR 4.0.1 to NSPR 4.1
because only a minor release can add new
functions.

Comment 4

[ruslan]

Hmm. It's not blocking any dogfood, no. But should we start using this for Socks 
or in-process ssl (which may be an option to fix MAC) - we'll need this. Is it a 
dangerous fix? Shouldn't be, right?

Comment 5

[Wan-Teh Chang]

This is not a dangerous fix, but I will not
check this into the NSPRPUB_CLIENT_BRANCH
(which is used by mozilla) unless it is
approved by the PDT.

Comment 6

[Wan-Teh Chang]

Attachment: Proposed patch.

Comment 7

[Wan-Teh Chang]

Checked in the patch (id=9391) on the main trunk.
/cvsroot/mozilla/nsprpub/pr/include/prio.h, revision 3.25
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c, revision 3.27
/cvsroot/mozilla/nsprpub/pr/src/io/priometh.c, revision 3.10
/cvsroot/mozilla/nsprpub/pr/src/io/prlayer.c, revision 3.12
/cvsroot/mozilla/nsprpub/pr/src/io/prpolevt.c, revision 3.10
/cvsroot/mozilla/nsprpub/pr/src/io/prsocket.c, revision 3.34
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptio.c, revision 3.53

Still need to add a test case.

Comment 8

[Wan-Teh Chang]

Fixed a bug in my previous checkin.  PR_GetConnectStatus
was passing the wrong fd to the connectcontinue method.
/cvsroot/mozilla/nsprpub/pr/src/io/prsocket.c, revision 3.37
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptio.c, revision 3.55

Comment 9

[Wan-Teh Chang]

I found that renaming the 'reserver_fn_4' member of PRIOMethods
to 'connectcontinue' breaks source compatibility.  I'm going to
attach a patch that renames 'connectcontinue' back to 'reserved_fn_4'.

Comment 10

[Wan-Teh Chang]

Attachment: Patch to rename the 'connectcontinue' member of PRIOmethods back to 'reserved_fn_4'.

Comment 11

[Wan-Teh Chang]

I checked in the patch.

/cvsroot/mozilla/nsprpub/pr/include/prio.h, revision: 3.30
/cvsroot/mozilla/nsprpub/pr/src/io/priometh.c, revision: 3.15
/cvsroot/mozilla/nsprpub/pr/src/io/prlayer.c, revision: 3.15
/cvsroot/mozilla/nsprpub/pr/src/io/prsocket.c, revision: 3.43
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptio.c, revision: 3.61

Comment 12

[Wan-Teh Chang]

I backed out the patch and reverted to 'connectcontinue'.
I convinced myself that users should treat the 'reserved_fn_x'
members of as if they didn't exist and shouldn't access those
members at all.  So renaming a 'reserved_fn_x' member to
'connectcontinue' is just like adding a new member, which does
not break source compatibility.

Comment 13

[larryh (gone)]

Marking as fixed.
Note: There is no checked in testcase for this fix.