Issue #395: Change client packet list to FIFO order

[esabol]

This PR addresses (I hope) issue #395. Just a draft for now.

[p-alik]

Code Review

Thanks for tackling this — the test structure and worker setup follow existing patterns well. A few things need to be addressed before this can land.

Critical Issues

1. The actual fix is missing

libgearman/packet.cc:191–198 still prepends to the list head (LIFO). The PR adds only a test; packet.cc is unchanged. Nothing here actually fixes the bug reported in issue #395.

2. The test asserts the wrong (broken) behavior

The assertion block is labelled /* === FIFO verification: wrong === */ and asserts LIFO ordering:

ASSERT_EQ('3', recorder.buffer[0]);   // expects 3 first — this is LIFO
ASSERT_EQ('2', recorder.buffer[1]);
ASSERT_EQ('1', recorder.buffer[2]);

The cout even says "LIFO packet order verified". Once the actual fix lands in packet.cc, this test will fail because the order will flip to 1, 2, 3. The assertions need to be inverted to verify FIFO ordering — which is what the PR title promises.

3. Client is never connected to a server

The test calls gearman_client_create(NULL) but never calls gearman_client_add_server(client, "localhost", context->port()). Without a server address, gearman_client_run_tasks() cannot submit anything — completion callbacks won't fire, recorder.pos will stay at 0, and ASSERT_EQ(3, recorder.pos) will fail.

---

Moderate Issues

4. Global mutable test state

recorder is a file-scope struct reset manually inside the test. If tests run in parallel or are repeated, this is a data race. Prefer a local variable passed through the task context pointer instead.

5. Commented-out debug code

//   ASSERT_TRUE(1 == 0); // temporary debug line - remove for final PR

The author noted it should be removed — shouldn't be in the diff at all, even commented out.

6. Missing error check on server connection

When gearman_client_add_server() is added, its return value should be checked — following the pattern used in the rest of client_test.cc.

---

What the packet.cc fix should look like

The fix needs to change from prepend-to-head (LIFO) to append-to-tail (FIFO). The cleanest approach is adding a packet_list_tail pointer to gearman_universal_st:

// FIFO: append to tail
packet.prev = universal.packet_list_tail;
packet.next = nullptr;
if (universal.packet_list_tail)
  universal.packet_list_tail->next = &packet;
else
  universal.packet_list = &packet;
universal.packet_list_tail = &packet;
universal.packet_count++;

The removal logic in gearman_packet_st::~gearman_packet_st() (~line 547) would also need to update packet_list_tail when the tail is removed.

---

Summary

Fix included	No
Test asserts correct (FIFO) behavior	No — asserts LIFO
Client connected to server	No
Debug leftovers	Yes

The PR is correctly marked as a draft. The main things needed before it can be reviewed for merge: the packet.cc fix itself, inverted test assertions ('1', '2', '3'), and the missing gearman_client_add_server() call.

[esabol]

The actual fix is missing

Um, yeah, I just dropped all those commits a couple hours ago in order to focus on getting the test working in an unchanged code state (with the order being LIFO). Getting a working test has been a challenge for me, obviously, and, at times, I wasn't sure if it was the test code or my changes to make the packet list FIFO. My plan is to get the test working first, then I'll modify the test to check FIFO and put the FIFO list changes back into place. I've stashed my changes away.