I think this is actually a bug in the demo poll server application. There is IMO missing a SSL_handle_events() call somewhere - either in the poll loop which might be unnecessary or at least after SSL_free() is called on the stream object.

Re-doing the SSL_shutdown() call implicitly in SSL_poll() seems wrong to me but what do others think? @mattcaswell @nhorman

I think this is actually a bug in the demo poll server application. There is IMO missing a SSL_handle_events() call somewhere - either in the poll loop which might be unnecessary or at least after SSL_free() is called on the stream object.

Re-doing the SSL_shutdown() call implicitly in SSL_poll() seems wrong to me but what do others think? @mattcaswell @nhorman

So in this case we need to update a documentation that event loop must call to SSL_poll() and SSL_handle_events().
the event loop used in demo tool reads as follows:

2840     while (pm->pm_event_count > 0) {
2841         ok = SSL_poll((SSL_POLL_ITEM *)pm->pm_poll_set, pm->pm_event_count,
2842                       sizeof (struct poll_event), &tv, 0, &poll_items);
2843
2844         if (ok == 0 && poll_items == 0)
2845             break;
2846
2847         for (i = 0; i < pm->pm_event_count; i++) {
2848             pe = &pm->pm_poll_set[i];
2849             e = 0;
2850             DPRINTFC(stderr, "%s %s (%p) " POLL_FMT "\n", __func__,
2851                      pe_type_to_name(pe), pe->pe_self,
2852                      POLL_PRINTA(pe->pe_poll_item.revents));
2853             pe->pe_self->pe_poll_item.revents = pe->pe_poll_item.revents;
2854             pe = pe->pe_self;
2855             if (pe->pe_poll_item.revents & SSL_POLL_ERROR)
2856                 e = pe->pe_cb_error(pe);
2857             else if (pe->pe_poll_item.revents & SSL_POLL_IN)
2858                 e = pe->pe_cb_in(pe);
2859             else if (pe->pe_poll_item.revents & SSL_POLL_OUT)
2860                 e = pe->pe_cb_out(pe);
2861             if (e == -1)
2862                 destroy_pe(pe);
2863         }
2864         rebuild_poll_set(pm);
2865         DPRINTFC(stderr, "%s ----------------------\n", __func__);
2866     }

The SSL_poll(3ossl) does not mention SSL_handle_events() at all. Also poll immediate document does not mention it at all. I would prefer fixing SSL_poll() as it is more intuitive approach. However I don't care I'll do whatever you feel it's right as long as I will understand what's going on there.

I'm afraid SSL_handle_events() won't help unless we extend it somehow. Because we need to handle locally triggered event SSL_shutdown() is being called by local application (client). The goal here is to make sure we call ossl_quic_channel_local_close(). Calling ossl_quic_channel_local_close() after the last stream is gone is the key detail of the proposed fix. The call to ossl_quic_channel_local_close() does not happen if there are stream still attached to connection.

If we want to fix application, then event loop must monitor connections which have shutdown in progress. For those connections the event loop must ensure to call SSL_shutdown() periodically.

I agree with @t8m that calling SSL_shutdown from within SSL_poll feels at least counter-intuitive, but as for whats going on, I can't currently say, as I've not looked closely enough at the code. @Sashan I think we were going to cover this tomorrow before your holiday yes? I'll try to look at this closely prior to that so I can speak intelligently about it

I'm afraid SSL_handle_events() won't help unless we extend it somehow. Because we need to handle locally triggered event SSL_shutdown() is being called by local application (client). The goal here is to make sure we call ossl_quic_channel_local_close(). Calling ossl_quic_channel_local_close() after the last stream is gone is the key detail of the proposed fix. The call to ossl_quic_channel_local_close() does not happen if there are stream still attached to connection.

If we want to fix application, then event loop must monitor connections which have shutdown in progress. For those connections the event loop must ensure to call SSL_shutdown() periodically.

Ah, you're right. So actually the application is supposed to repeat SSL_shutdown() calls once it intends to shutdown the connection. See this excerpt from the manpage https://docs.openssl.org/master/man3/SSL_shutdown/#shutdown-mode :

RFC compliant shutdown mode

This is the default behaviour. The shutdown process may take a period of time up to three times the current estimated RTT to the peer. It is possible for the closure process to complete much faster in some circumstances but this cannot be relied upon.

In blocking mode, the function will return once the closure process is complete. In nonblocking mode, SSL_shutdown_ex() should be called until it returns 1, indicating the closure process is complete and the connection is now fully shut down.

connections the event loop must ensure to call SSL_shutdown() periodically.

Ah, you're right. So actually the application is supposed to repeat SSL_shutdown() calls once it intends to shutdown the connection. See this excerpt from the manpage https://docs.openssl.org/master/man3/SSL_shutdown/#shutdown-mode :

RFC compliant shutdown mode

This is the default behaviour. The shutdown process may take a period of time up to three times the current estimated RTT to the peer. It is possible for the closure process to complete much faster in some circumstances but this cannot be relied upon.

In blocking mode, the function will return once the closure process is complete. In nonblocking mode, SSL_shutdown_ex() should be called until it returns 1, indicating the closure process is complete and the connection is now fully shut down.

yes exactly. The problem is the non-blocking variant of SSL_shutdown() does not play well with blocking SSL_poll(). The SSL_poll() allows application to ask for indefinite blocking. Therfore I think the proposed change makes kind of sense. The SSL_poll() can do a courtesy for polling application to call SSL_shutdown() on SSL connection objects which have a shutdown operation in progress (the shutdown waits for streams to complete the transfer).

``. The SSL_poll() allows application to ask for indefinite blocking.

This is an application bug. An application must use SSL_get_event_timeout() to set the timeout for SSL_poll()

``. The SSL_poll() allows application to ask for indefinite blocking.

This is an application bug. An application must use SSL_get_event_timeout() to set the timeout for SSL_poll()

In this case SSL_poll() API should not allow to set indefinite time out all. I think. IMO we should prevent applications to pass timeout argument to SSL_poll() being NULL. Another option would be to adjust PR 27441, I think it can be modified to send _EC event to let application know it's time to re-issue SSL_shutdown() event on the given connection.

With current state of affairs allowing indefinite poll just allows applications to shoot their knee when they need to use SSL_shutdown(). SSL_shitdown() just does not play well with indefinite SSL_poll(). And it's nowhere documented.

I'd like to come up with a solution that creates the least surprises for applications that use SSL_poll().

I agree this a bug in application. The question is how it should get fixed:

• leave things as they are and document it
• opt for the change here (and document it)

to be honest I'm not able to think of any drawback in current approach. It seems to work well with SSL_poll() that blocks indefinitely.