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.

Sign up for GitHub

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

Jump to bottom

Expose the ServerConnection created timestamp #801

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Expose the ServerConnection created timestamp #801

wants to merge 2 commits into from

Conversation

eagleshine
Copy link

No description provided.

@undertow-pull-request
Copy link

Can one of the admins verify this patch?

@fl4via
Copy link
Member

fl4via commented Aug 30, 2019

This is ok to test

@fl4via
Copy link
Member

fl4via commented Aug 30, 2019

Hi @eagleshine ! Thanks for your PR!
Undertow is not compiling, the following error occurs: Compilation failure /store/work/tc-work/b455211ade1ff869/core/src/main/java/io/undertow/server/protocol/http2/Http2ServerConnection.java:[83,8] io.undertow.server.protocol.http2.Http2ServerConnection is not abstract and does not override abstract method getCreatedTimestamp() in io.undertow.server.ServerConnection
Can you fix it?
Also, what is the drive behing the timestamp exposition? Are you using that for some logging or something?

@fl4via fl4via added the waiting PR update Awaiting PR update(s) from contributor before merging label Aug 30, 2019
@eagleshine
Copy link
Author

We have an HTTPS server that has some logic to decide when to return the header "Connection: close" to the client using the connection created timestamp. We are using some internally developed web framework but want to migrate to either Jetty or Undertow.

carterkozak
carterkozak reviewed Aug 30, 2019
View reviewed changes
core/src/main/java/io/undertow/server/protocol/http2/Http2ServerConnection.java
@@ -110,6 +111,7 @@ public Http2ServerConnection(Http2Channel channel, Http2StreamSourceChannel requ
originalSourceConduit = new StreamSourceChannelWrappingConduit(requestChannel);
this.conduitStreamSinkChannel = new ConduitStreamSinkChannel(responseChannel, originalSinkConduit);
this.conduitStreamSourceChannel = new ConduitStreamSourceChannel(channel, originalSourceConduit);
this.createdTimestamp = System.currentTimeMillis();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? If memory serves, a new Http2ServerConnection object is created for each request, so the createdTimestamp will be set at the same time as HttpServerExchange.requestStartTime (which uses the high resolution nanoTime, and is default disabled for performance).

Copy link
Author

Choose a reason for hiding this comment

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

OK. Looks like the semantics of the Connection changed in Http2: the equivalent to that of Http in Http2 is Http2Channel. And you're right that a new Http2ServerConnection object is created for each request, however, this is still the connection created timestamp, just not useful anymore.

Copy link
Member

Choose a reason for hiding this comment

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

@eagleshine does that mean that this PR won't work the way you intended it to? If it is no longer valid, please close it. If that's not the case, please let me know

@fl4via fl4via added the failed CI Introduced new regession(s) during CI check label Sep 1, 2019
@fl4via fl4via added under verification Currently being verified (running tests, reviewing) before posting a review to contributor and removed waiting PR update Awaiting PR update(s) from contributor before merging labels Oct 15, 2019
@fl4via fl4via added question Waiting for additional information from contributor and removed failed CI Introduced new regession(s) during CI check under verification Currently being verified (running tests, reviewing) before posting a review to contributor labels Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carterkozak carterkozak carterkozak left review comments

@fl4via fl4via fl4via left review comments

Assignees
No one assigned
Labels
question Waiting for additional information from contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eagleshine @undertow-pull-request @fl4via @carterkozak