888581 - Allow integrating NSPR/NSS sockets into non-NSPR event loops

Status: REOPENED

(NSPR :: NSPR, defect)

Description

[Miloslav Trmač]

Attachment: Add PR_SocketPollingHandle()

Event-driven applications that want to use NSS for TLS can currently do so only by designing the event loop around PR_Poll(), because NSS needs to be able to remap PR_POLL_{READ,WRITE} to reflect e.g. the actual direction necessary for performing renegotiations; this remapping (through a PRPollFN) happens only implicitly within PR_Poll() and is not otherwise available to applications.

Changing the primary event loop is an extremely invasive requirement; it makes it impossible to use NSS together with software assuming a different event loop (e.g. the one in GLib), or to encapsulate NSS use in a module without requiring users of the module to know about NSS.


To allow using NSPR sockets in other event loops built around select(), poll() or similar calls, applications need
1. the handle to use
2. a way to remap the flags.

This is essentially the mirror of PR_{Create,Destroy}SocketPollFd()


1. The handle is currently available through PR_FileDesc2NativeHandle(), which is in a "private" header file.  I'll attach a patch that proposes a more limited version, PR_SocketPollingHandle(), documented in a way that should avoid the UNIX/Windows semantics differences.

2. The flags are available by directly calling fd->methods->poll; proposed PR_SocketPollingStatus() both encapsulates this and makes the semantics documented.


The patches don't do anything fundamentally new, they would just allow callers to use officially supported and documented interfaces instead of private undocumented ones.

There certainly are alternative approaches - e.g.
1. Just formally bless PR_FileDesc2NativeHandle() and leave it up to the caller to be careful about non-sockets.
2a. Add PR_NativePollingStatus() that returns <poll.h> POLL* flags instead of PR_POLL_*, making it easier for UNIX callers to map the flags.
2b. In addition to PR_SocketPollingStatus(), add also PR_SocketPollingResult() that helps mapping back from the poll() direction returned on the underlying socket to the application-relevant I/O direction.

Comment 1

[Miloslav Trmač]

Attachment: Add PR_SocketPollingStatus()

Comment 2

[Robert Relyea]

Comment on attachment 769341 [details] [diff] [review]
Add PR_SocketPollingHandle()

r+

My only reservation is ptio.h. I think we should probably return -1 if pthreads are used. In practice I don't think this is an issue as I don't think we have any more platforms that no longer support native threads.

wtc do you have an opinion?

Comment 3

[Robert Relyea]

Comment on attachment 769342 [details] [diff] [review]
Add PR_SocketPollingStatus()

r+ rrelyea.

Unlike the other function, since this uses the underlying poll function it should be safe for the ptio case.

Comment 4

[Kai Engert (:KaiE:)]

both patches checked in:
https://hg.mozilla.org/projects/nspr/rev/1bd2f00745d3
https://hg.mozilla.org/projects/nspr/rev/77c4ee30c1fe

Comment 5

[Wan-Teh Chang]

Comment on attachment 769341 [details] [diff] [review]
Add PR_SocketPollingHandle()

Review of attachment 769341 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. I prefer just use the existing PR_FileDesc2NativeHandle
function.

::: pr/src/io/prsocket.c
@@ +1552,5 @@
> +PR_SocketPollingHandle(PRFileDesc *fd)
> +{
> +    if (!_pr_initialized) _PR_ImplicitInitialization();
> +    /* Should this refuse a non-socket fd? */
> +    return PR_FileDesc2NativeHandle(fd);

Why don't you use PR_FileDesc2NativeHandle? In general there is nothing
wrong with using a function from "private/pprio.h" because the header is
exported. The "private" directory name is unfortunate. It is similar to
the "friend" access in C++.

Comment 6

[Wan-Teh Chang]

Comment on attachment 769342 [details] [diff] [review]
Add PR_SocketPollingStatus()

Review of attachment 769342 [details] [diff] [review]:
-----------------------------------------------------------------

::: pr/include/prio.h
@@ +2057,5 @@
> + *       should wait for on the underlying OS socket.  On failure, the value
> + *       is -1.
> + ************************************************************************
> + */
> +NSPR_API(PRInt32) PR_SocketPollingStatus(PRFileDesc *fd, PRInt16 in_flags,

We should remove "Socket" from the function name because the |fd|
can also be other kinds of file descriptors, for example, pipes
or even regular files.

Just wanted to confirm: this function is just syntactic sugar, right?
Because you can already invoke the |poll| method directly.

Comment 7

[Kai Engert (:KaiE:)]

Reopening, because we want to get a NSPR release completed, but Wan-Teh would like more time to properly review the function signatures etc.

I've temporarily backed out the patches for the release process.

https://hg.mozilla.org/projects/nspr/rev/b3bb2fe7f716
https://hg.mozilla.org/projects/nspr/rev/4e13f425d282

Comment 8

[Kai Engert (:KaiE:)]

I understand that Miloslav needs to respond to Wan-Teh's comment, prior to proceeding with this work. I'll postpone relanding the patch until there's progress and agreement on the API.

Comment 9

[Wan-Teh Chang]

Comment on attachment 769341 [details] [diff] [review]
Add PR_SocketPollingHandle()

Review of attachment 769341 [details] [diff] [review]:
-----------------------------------------------------------------

Miloslav: thank you very much for the patch. NSPR users should
use PR_FileDesc2NativeHandle() for this purpose.

Some headers in NSPR's "private" directory are exported, public
headers. "private/pprio.h" is such a header. The "private"
directory name comes from the NSPR-only days. In a mixed
NSPR/native environment, some of the functions declared in these
headers are crucial.

So, please feel free to use PR_FileDesc2NativeHandle(). Do you
have any suggestion on changes to the comments in "private/pprio.h"
to clarify this issue?

::: pr/src/pthreads/ptio.c
@@ +4633,5 @@
>  }  /* PR_FileDesc2NativeHandle */
>  
> +PR_IMPLEMENT(PROsfd) PR_SocketPollingHandle(PRFileDesc *fd)
> +{
> +    if (!_pr_initialized) _PR_ImplicitInitialization();

This _PR_ImplicitInitialization() call is not necessary because the PRFileDesc input argument implies NSPR is already initialized.

Comment 10

[Wan-Teh Chang]

Comment on attachment 769342 [details] [diff] [review]
Add PR_SocketPollingStatus()

Review of attachment 769342 [details] [diff] [review]:
-----------------------------------------------------------------

Miloslav: thank you for the patch.

It is OK to call fd->methods->poll directly. It seems useful to
add a PR_SocketPollingStatus function to clarify it's legal to
call the fd->methods->poll method.

fd->methods->poll was originally intended for internal use by
PR_Poll. But it is now crucial for interfacing to a non-NSPR event
polling system.

I suggest renaming this function "PR_GetPollingStatus" or
"PR_GetPollStatus" because 1) a function name should have a verb,
and 2) the function can also be used on pipes on Unix.

::: pr/include/prio.h
@@ +2057,5 @@
> + *       should wait for on the underlying OS socket.  On failure, the value
> + *       is -1.
> + ************************************************************************
> + */
> +NSPR_API(PRInt32) PR_SocketPollingStatus(PRFileDesc *fd, PRInt16 in_flags,

Please declare this function after PR_SendTo. This matches the ordering of the sendto and poll methods in the PRIOMethods structure.

::: pr/src/pthreads/ptio.c
@@ +4645,4 @@
>      if (fd) fd->secret->md.osfd = handle;
>  }  /*  PR_ChangeFileDescNativeHandle*/
>  
> +PR_IMPLEMENT(PRInt32) PR_SocketPollingStatus(PRFileDesc *fd, PRInt16 in_flags,

This function should be defined, once, in nspr/pr/src/io/priometh.c. Define it after PR_SendTo.

@@ +4647,5 @@
>  
> +PR_IMPLEMENT(PRInt32) PR_SocketPollingStatus(PRFileDesc *fd, PRInt16 in_flags,
> +    PRInt16 *out_flags)
> +{
> +    if (!_pr_initialized) _PR_ImplicitInitialization();

This _PR_ImplicitInitialization() call is also not necessary because the PRFileDesc input argument implies NSPR is already initialized.

Comment 11

[Wan-Teh Chang]

Attachment: Add PR_GetPollingFlags()

Miloslav: could you review this patch? Thanks.

I think "PR_GetPollingFlags" may be a good name for this
function. Let me know what you think of the function name
and the comments. I included a link to the NSPR article
"NSPR Poll Method".

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.