PROTON-2748: Raw connection async close fix and tests. #402

cliffjansen · 2023-08-15T05:25:46Z

No description provided.

astitcher

Doesn't compile for me - and the compiler found a definite error in the code.

c/src/proactor/epoll_raw_connection.c

…ator

astitcher

A bunch of things to think about here. Probably nothing that significant, but these might make it a bit easier for me to understand and follow the code.
I wonder if the filename raw_wake_test should be raw_connection_wake_test for consistency?

astitcher · 2023-08-28T16:14:35Z

c/src/proactor/epoll_raw_connection.c

@@ -304,7 +305,7 @@ void pn_raw_connection_write_close(pn_raw_connection_t *rc) {
 pni_raw_write_close(rc);
 }

-static pn_event_t *pni_raw_batch_next(pn_event_batch_t *batch) {
+static pn_event_t *pni_epoll_raw_batch_next(pn_event_batch_t *batch, bool peek_only) {


rename this: pni_raw_batch_next_common() or pni_raw_batch_next_or_peek():
Ading epoll to the name doesn't communicate anything useful about the purpose of this function.

pni_raw_batch_has_events() was used as suggested elsewhere.

astitcher · 2023-08-28T16:17:49Z

c/src/proactor/epoll_raw_connection.c

- if (!e || pn_event_type(e) == PN_RAW_CONNECTION_DISCONNECTED)
- rc->batch_empty = true;


Where did this logic go? It seems no longer to be anywhere. Is it no longer needed? Specifically the check against the disconnected event.

This logic was solely there to identify edge cases where the epoll-specific code could not be sure if the state machine was up to date. In which case a non-application call to pni_raw_batch_next() would update the state machine but a resulting event would have to be put back into the collector.

The new pni_raw_batch_has_events() updates the state machine and has no side effects to the event stream, so the check for batch_empty is no longer needed.

astitcher · 2023-08-28T16:27:10Z

c/src/proactor/epoll_raw_connection.c

- if (events & EPOLLIN) pni_raw_read(&rc->raw_connection, fd, rcv, set_error);
- if (events & EPOLLOUT) pni_raw_write(&rc->raw_connection, fd, snd, set_error);
- rc->batch_empty = false;
+ if (rc->connected) {


I don't think you need this condition (which is why it wasn't present previously) - if you get here without returning already you must be connected.

astitcher · 2023-08-28T16:31:20Z

c/src/proactor/epoll_raw_connection.c

+ pni_raw_async_disconnect(&rc->raw_connection);
+ } else if (events & EPOLLHUP) {


The style of this function is early return. So please change this to have a return with no 'else if' if that is possible - I don't understand enough of this logic to see if you could get EPOLLERR as well as EPOLLIN, EPOLLOUT or EPOLLRDHUP at the same time with a useful action of reading or writing, but it seems unlikely to me.
And if this change is possible it lowers the cognitive load (and indentation) of the function.

astitcher · 2023-08-28T16:43:01Z

c/src/proactor/epoll_raw_connection.c

+static pn_event_t *pni_raw_batch_peek(pn_event_batch_t *batch) {
+ return pni_epoll_raw_batch_next(batch, true);
+}


Are you sure you want to introduce a peek operation? This is only ever used to find out whether there is an outstanding event so why not just introduce an operation bool pni_raw_batch_has_events()?

astitcher · 2023-08-28T16:48:00Z

c/src/proactor/epoll_raw_connection.c

- // If we're closed and we've sent the disconnect then close
- pni_raw_finalize(raw);
- praw_connection_cleanup(rc);
+ // Must poll for iO.


astitcher · 2023-08-28T17:07:43Z

c/src/proactor/raw_connection.c

+pn_event_t *pni_raw_event_peek(pn_raw_connection_t *conn) {
+ return pni_get_next_raw_event(conn, true);
+}
+


I think I would refactor pni_raw_event_next differently:

Refactor the event generating if chain into a separate function that is called by ...next() and ...peek() (or if you follow my earlier suggestion ...has_events()

Then wrap that with pn_collector_next/peek logic.

Just in case it's not clear the do ... while (true) is a bit of a red herring and somewhat bad coding on my part in that there is never more than one effective pass through the loop. There can be 2 but that if only if there was no initial event. So it make more sense really to separate out the event generating logic.

Hope that is clear enough.

done with has_events().

astitcher · 2023-08-28T17:10:32Z

c/src/proactor/raw_connection-internal.h

@@ -134,9 +134,11 @@ void pni_raw_write_close(pn_raw_connection_t *conn);
 void pni_raw_read(pn_raw_connection_t *conn, int sock, long (*recv)(int, void*, size_t), void (*set_error)(pn_raw_connection_t *, const char *, int));
 void pni_raw_write(pn_raw_connection_t *conn, int sock, long (*send)(int, const void*, size_t), void (*set_error)(pn_raw_connection_t *, const char *, int));
 void pni_raw_process_shutdown(pn_raw_connection_t *conn, int sock, int (*shutdown_rd)(int), int (*shutdown_wr)(int));
+void pni_raw_async_disconnect(pn_raw_connection_t *conn);


As this is a new event going to the raw connection state machine maybe there should be a new test for this event in raw_connection_test.

… return

astitcher · 2023-09-08T21:15:03Z

@cliffjansen I really like how this has turned out now. Are you finished or are there more changes due to adding new tests?

cliffjansen · 2023-09-11T17:14:24Z

I will check in the changes so far to get some extended CI going.

Todo: additional tests as suggested and rename the test. I will leave the pull and JIRA open until they are done and will incorporate additional suggestions.

I propose changing raw_wake_test to raw_connection_proactor_test to reflect it tests the functionality of the proactor implementation of raw connections whereas the existing raw_connection_test tests the proactor-independent bits of the internal/external API without a running proactor.

…proactor_test

astitcher · 2023-09-22T17:21:50Z

The new commits lgtm

PROTON-2748: Raw connection async close fix and tests.

f73813a

astitcher requested changes Aug 24, 2023

View reviewed changes

c/src/proactor/epoll_raw_connection.c Outdated Show resolved Hide resolved

PROTON-2748: pull request 402 review - replace incorrect boolean oper…

7c6fc14

…ator

astitcher requested changes Aug 28, 2023

View reviewed changes

cliffjansen added 2 commits September 6, 2023 17:01

PROTON-2748: pull request 402 review - peek->has_events

63985a9

PROTON-2748: pull request 402 review - isolate !connected code, early…

4529406

… return

cliffjansen added 2 commits September 21, 2023 09:56

PROTON-2748: pull request 402 review - internal api tests

546f046

PROTON-2748: pull request 402 review - rename test to raw_connection_…

9c83182

…proactor_test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PROTON-2748: Raw connection async close fix and tests. #402

PROTON-2748: Raw connection async close fix and tests. #402

cliffjansen commented Aug 15, 2023

astitcher left a comment

astitcher left a comment

astitcher Aug 28, 2023

cliffjansen Sep 8, 2023

astitcher Aug 28, 2023

cliffjansen Sep 8, 2023

astitcher Aug 28, 2023

cliffjansen Sep 8, 2023

astitcher Aug 28, 2023

cliffjansen Sep 8, 2023

astitcher Aug 28, 2023

cliffjansen Sep 8, 2023

astitcher Aug 28, 2023

cliffjansen Sep 8, 2023

astitcher Aug 28, 2023

cliffjansen Sep 8, 2023

astitcher Aug 28, 2023

astitcher commented Sep 8, 2023

cliffjansen commented Sep 11, 2023

astitcher commented Sep 22, 2023

		if (!e \|\| pn_event_type(e) == PN_RAW_CONNECTION_DISCONNECTED)
		rc->batch_empty = true;

		pni_raw_async_disconnect(&rc->raw_connection);
		} else if (events & EPOLLHUP) {

PROTON-2748: Raw connection async close fix and tests. #402

Are you sure you want to change the base?

PROTON-2748: Raw connection async close fix and tests. #402

Conversation

cliffjansen commented Aug 15, 2023

astitcher left a comment

Choose a reason for hiding this comment

astitcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astitcher commented Sep 8, 2023

cliffjansen commented Sep 11, 2023

astitcher commented Sep 22, 2023