RdmaServerEndpoint accept may return null, if listening endpoint closes.

[BernardMetzler]

Closing the RdmaServerEndpoint in a current accept() lets a poll of the private accepting endpoint queue return null. This patch just avoids dereferencing that null endpoint during accept(). The caller of RdmaServerEndpoint.accept() must handle that case appropriately.

[PepperJo]

Are we sure applications are handling the case correctly if null is returned here? Might as well throw an exception (Much like before)?

[BernardMetzler]

-----"Jonas Pfefferle" <notifications@github.com> wrote: -----

To: "zrlio/disni" ***@***.***> From: "Jonas Pfefferle" ***@***.***> Date: 10/05/2020 08:29AM Cc: "Bernard Metzler" ***@***.***>, "Author" ***@***.***> Subject: [EXTERNAL] Re: [zrlio/disni] RdmaServerEndpoint accept may return null, if listening endpoint closes. (#54) @PepperJo requested changes on this pull request. In src/main/java/com/ibm/disni/RdmaServerEndpoint.java: > @@ -130,8 +130,12 @@ public C accept() throws IOException { } } C endpoint = requested.poll(); - logger.info("connect request received"); - endpoint.accept(); + + // a null endpoint gets returned if listening endpoint closes + if (endpoint != null) { + logger.info("connect request received"); + endpoint.accept(); + } return endpoint; Are we sure applications are handling the case correctly if null is returned here? Might as well throw an exception (Much like before)?

No we are not sure. The patch would change the semantics of a server endpoint, and applications would have to be aware of it. We run into it, if we are terminating a server endpoint, which is still in its accept call. Do we want to crash here, or give the application a chance to handle that gracefully...

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[PepperJo]

But we don't really crash, we just throw an exception which can be gracefully handled. If we want the application to be able to distinguish we should make sure to throw a different exception type. On Mon, 05 Oct 2020 07:39:19 -0700 Bernard Metzler <notifications@github.com> wrote:

…

-----"Jonas Pfefferle" ***@***.***> wrote: ----- >To: "zrlio/disni" ***@***.***> >From: "Jonas Pfefferle" ***@***.***> >Date: 10/05/2020 08:29AM >Cc: "Bernard Metzler" ***@***.***>, "Author" ***@***.***> >Subject: [EXTERNAL] Re: [zrlio/disni] RdmaServerEndpoint accept may >return null, if listening endpoint closes. (#54) > > > @PepperJo requested changes on this pull request. > In src/main/java/com/ibm/disni/RdmaServerEndpoint.java: > > @@ -130,8 +130,12 @@ public C accept() throws IOException { > } > } > C endpoint = requested.poll(); >- logger.info("connect request received"); >- endpoint.accept(); >+ >+ // a null endpoint gets returned if listening endpoint closes >+ if (endpoint != null) { >+ logger.info("connect request received"); >+ endpoint.accept(); >+ } > return endpoint; > Are we sure applications are handling the case correctly if null is >returned here? Might as well throw an exception (Much like before)? No we are not sure. The patch would change the semantics of a server endpoint, and applications would have to be aware of it. We run into it, if we are terminating a server endpoint, which is still in its accept call. Do we want to crash here, or give the application a chance to handle that gracefully... > — >You are receiving this because you authored the thread. >Reply to this email directly, view it on GitHub, or unsubscribe. > -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #54 (comment)