Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

H2 minor fixes #4246

Merged
merged 2 commits into from
Jan 3, 2025
Merged

H2 minor fixes #4246

merged 2 commits into from
Jan 3, 2025

Conversation

walid-git
Copy link
Member

Two minor fixes, see commit logs for details.

We should never be scheduled when on H2_S_CLOSED state.
For PU upgrade, at the time of this log, the req->vsl is not assigned a vxid
yet, and thus the line would be logged for vxid 0, which doen't make much
sense. In all cases, it is better to log it on the sess vsl instead.

Noticed while reading a vtc output.
Copy link
Member

@nigoroll nigoroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable, but I do not feel confident to say for sure if 93f806b is correct

@walid-git
Copy link
Member Author

looks reasonable, but I do not feel confident to say for sure if 93f806b is correct

We only transition to H2_S_CLOSED state in two places, and we set r2->scheduled to 0 in both:

$ grep -rniI "H2_S_CLOSED" -C 4
bin/varnishd/http2/cache_http2_proto.c-625-		h2 = r2->h2sess;
bin/varnishd/http2/cache_http2_proto.c-626-		CHECK_OBJ_NOTNULL(h2, H2_SESS_MAGIC);
bin/varnishd/http2/cache_http2_proto.c-627-		Lck_Lock(&h2->sess->mtx);
bin/varnishd/http2/cache_http2_proto.c-628-		r2->scheduled = 0;
bin/varnishd/http2/cache_http2_proto.c:629:		r2->state = H2_S_CLOSED;
bin/varnishd/http2/cache_http2_proto.c-630-		r2->h2sess->do_sweep = 1;
bin/varnishd/http2/cache_http2_proto.c-631-		Lck_Unlock(&h2->sess->mtx);
bin/varnishd/http2/cache_http2_proto.c-632-	}
bin/varnishd/http2/cache_http2_proto.c-633-	THR_SetRequest(NULL);
--
bin/varnishd/http2/cache_http2_proto.c-715-	req->task->priv = req;
bin/varnishd/http2/cache_http2_proto.c-716-	r2->scheduled = 1;
bin/varnishd/http2/cache_http2_proto.c-717-	if (Pool_Task(wrk->pool, req->task, TASK_QUEUE_STR) != 0) {
bin/varnishd/http2/cache_http2_proto.c-718-		r2->scheduled = 0;
bin/varnishd/http2/cache_http2_proto.c:719:		r2->state = H2_S_CLOSED;
bin/varnishd/http2/cache_http2_proto.c-720-		return (H2SE_REFUSED_STREAM); //rfc7540,l,3326,3329
bin/varnishd/http2/cache_http2_proto.c-721-	}
bin/varnishd/http2/cache_http2_proto.c-722-	return (0);
bin/varnishd/http2/cache_http2_proto.c-723-}
--
bin/varnishd/http2/cache_http2_proto.c-1413-			assert (r2->state == H2_S_IDLE);
bin/varnishd/http2/cache_http2_proto.c-1414-			continue;
bin/varnishd/http2/cache_http2_proto.c-1415-		}
bin/varnishd/http2/cache_http2_proto.c-1416-		switch (r2->state) {
bin/varnishd/http2/cache_http2_proto.c:1417:		case H2_S_CLOSED:
bin/varnishd/http2/cache_http2_proto.c-1418-			AZ(r2->scheduled);
bin/varnishd/http2/cache_http2_proto.c-1419-			h2_del_req(wrk, r2);
bin/varnishd/http2/cache_http2_proto.c-1420-			break;
bin/varnishd/http2/cache_http2_proto.c-1421-		case H2_S_CLOS_REM:

@nigoroll nigoroll merged commit df7d3ac into varnishcache:master Jan 3, 2025
9 of 10 checks passed
@nigoroll
Copy link
Member

nigoroll commented Jan 3, 2025

@walid-git with that compelling argument we do not need much of a review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants