remove request from seq_list when errors occur

[i-rinat]

Adds missing error handling. If a request cannot be forwarded, it's dropped. In that case we need to remove it from the connection's queue.

(fixes #1099)

[krizhanovsky]

Probably the patch is good, just a couple of questions since we've been fixing bugs in the code for a while.

[krizhanovsky]

It was supposed that tfw_http_resp_fwd() takes care about the responses and linked requests in the final loop if !list_empty(&ret_queue)) - why do we need to free requests and responses here? Don't we need to adjust or completely remove the loop at the end of tfw_http_resp_fwd()?

[vankoven]

Correct, tfw_http_resp_fwd() takes care on unforwarded responses, and all the responses are destroyed there. The new line list_del_init(&req->msg.seq_list); was missed there thought, so #1099 happen. This exact lines are not required here since we still do the same later during error handling at the end of tfw_http_resp_fwd() .

[i-rinat]

why do we need to free requests and responses here? Don't we need to adjust or completely remove the loop at the end of tfw_http_resp_fwd()?

It's possible to move all the lines from the loop to __tfw_http_resp_fwd(), but there will be no simplification other than moving all reclamation code together. Now two sites are apart from each another. That's slightly confusing. However, that much is fine.

[krizhanovsky]

Do we need the loop if we deleted all the requests in the patch above? Maybe it makes sense to write 2 lines instead of 4?

tfw_http_conn_msg_free(req->pair);
tfw_http_conn_req_clean(req);

[i-rinat]

After @ikoveshnikov's comment, I realized there is no need to lock, as code above extracts a part of the list. That part is now private, so here only a single list_del_init() is required, without locking. There is no need to check whenever req->msg.seq_list is empty — it's not, since we get req by iterating over a list with req->msg.seq_list in it. So tfw_http_conn_req_clean() is unsuitable here.

[vankoven]

The patch adds duplicated code and unnecessary spinlock operations, which should be removed.

[vankoven]

Correct, tfw_http_resp_fwd() takes care on unforwarded responses, and all the responses are destroyed there. The new line list_del_init(&req->msg.seq_list); was missed there thought, so #1099 happen. This exact lines are not required here since we still do the same later during error handling at the end of tfw_http_resp_fwd() .

[vankoven]

The list passed to __tfw_http_resp_fwd() is not cli_conn->seq_queue. The ret_queue is cut from cli_conn->seq_queue:

tempesta/tempesta_fw/http.c

Line 2305 in 774869c

__list_cut_position(&ret_queue, seq_queue, req_retent);

So the req is not a member of cli_conn->seq_queue, and the spinlock is not required here.

[i-rinat]

Indeed. Removed.

[i-rinat]

Updated patch according to the comments. Now it's just single list_del_init(&req->msg.seq_list);.

[aleksostapenko]

Nothing to add. Good to merge.

[vankoven]

Good to merge!