Fix #2286 HTTP/2 communication may return HTTP 400

[11rx4f]

• Fixes HTTP/2 communication may return HTTP 400 #2286

Fixes the HTTP/2 connection preface validation to prevent PRI-specific validation from being executed multiple times.

Signed-off-by: 11rx4f ryosuke.okada@fujitsu.com

[carryel]

@11rx4f Thank you for submitting the PR! Could you possibly add relevant test cases for this issue as well?

[11rx4f]

@carryel Sure, I’ll add the relevant test cases.

[pzygielo]

No other field here is volatile. Why this one should be?

[carryel]

Additionally, have you considered putting that property into Http2Session? The reason I thought of this is that Http2Session already manages similar states, such as isPrefaceReceived.

[11rx4f]

@pzygielo I made this field volatile for better safety and visibility in a potential concurrent scenario.
Do you think it would also make sense to apply volatile to the other fields?

@carryel I did consider putting this property into Http2Session.
However, since this flag is intended to suppress PRI re-validation before the HTTP/2 session is established,
I thought it might fit more naturally in Http2State, which handles connection-phase state.

[pzygielo]

Do you think it would also make sense to apply volatile to the other fields?

No. To drop it from this one.

[carryel]

2. As @pzygielo mentioned, it does not need to be declared as volatile.

In contrast, Http2Session's isPrefaceReceived is declared as volatile, but this is likely due to multi-thread checks between handleRead() and handleEvent().

It seems that the newly defined Http2State's isPriReceived does not need to be declared as volatile. Cases where handleRead() is called simultaneously by multiple threads on the same connection will not occur.

[carryel]

If http2State.isPriReceived() is false, wouldn't http2Session.isHttp2InputEnabled() always be false as well?

[11rx4f]

Thanks for pointing that out. When isPriReceived is false, isHttp2InputEnabled is also false in this implementation. They still represent different states: isPriReceived is about PRI validation completion, while isHttp2InputEnabled is about full handshake readiness.

[carryel]

I would like to recommend two things. Please take a look at my suggestion as well.

1. We have separated the logic for Preface received and the logic for PRI checked. Since Preface is always 'not received' if PRI is not checked, we have clearly separated these logics.

        if (!http2Session.isHttp2InputEnabled()) { // Preface is not received yet
            if (http2State.isHttpUpgradePhase()) {
                // It's plain HTTP/1.1 data coming with upgrade request
                if (httpContent.isLast()) {
                    http2State.setDirectUpgradePhase(); // expecting preface
                    enableOpReadNow(ctx);
                }
                return ctx.getInvokeAction();
            }
        }
        final Buffer framePayload;
        if (!http2State.isPriReceived()) { // PRI is not checked yet
            final HttpRequestPacket httpRequest = (HttpRequestPacket) httpHeader;

            // PRI message hasn't been checked
            try {
                if (!checkPRI(httpRequest, httpContent)) {
                    // Not enough PRI content read
                    return ctx.getStopAction(httpContent);
                }
                http2State.setPriReceived(true);
            } catch (Exception e) {
                httpRequest.getProcessingState().setError(true);
                httpRequest.getProcessingState().setKeepAlive(false);

                final HttpResponsePacket httpResponse = httpRequest.getResponse();
                httpResponse.setStatus(HttpStatus.BAD_REQUEST_400);
                ctx.write(httpResponse);
                connection.closeSilently();

                return ctx.getStopAction();
            }

            final Buffer payload = httpContent.getContent();
            framePayload = payload.split(payload.position() + PRI_PAYLOAD.length);
        } else {
            framePayload = httpContent.getContent();
        }

[11rx4f]

FYI, this review was started unintentionally while replying.
Please treat the comments as regular discussion points.

[11rx4f]

@pzygielo I made this field volatile for better safety and visibility in a potential concurrent scenario.
Do you think it would also make sense to apply volatile to the other fields?

@carryel I did consider putting this property into Http2Session.
However, since this flag is intended to suppress PRI re-validation before the HTTP/2 session is established,
I thought it might fit more naturally in Http2State, which handles connection-phase state.

[11rx4f]

Thanks for pointing that out. When isPriReceived is false, isHttp2InputEnabled is also false in this implementation. They still represent different states: isPriReceived is about PRI validation completion, while isHttp2InputEnabled is about full handshake readiness.

[11rx4f]

I’ve added the relevant test cases.

[carryel]

I have given you two recommendations regarding the PR you submitted.
Please take another look at this.
Thanks!

[11rx4f]

Thanks for the comments.
I’ve addressed the two points you mentioned.