Closed Bug 40778 Opened 20 years ago Closed 19 years ago

Support for layered nonblocking connect

Categories

(NSPR :: NSPR, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

References

Details

Attachments

(2 files)

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.)
Status: NEW → ASSIGNED
Target Milestone: --- → 4.1
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?
Sounds cool to me. We need to make sure that the new nspr gets into mozilla 
builds once this is fixed
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.
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?
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.
Attached patch Proposed patch.Splinter Review
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.
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
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'.
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
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.
Marking as fixed.
Note: There is no checked in testcase for this fix.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
No longer blocks: 331169
Depends on: 331169
No longer depends on: 868803
You need to log in before you can comment on or make changes to this bug.