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

Reorganization of jetty-client classes. #9127

Merged
merged 4 commits into from
Jan 11, 2023

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Jan 6, 2023

  • Moved oej.client.api to oej.client

  • Moved oej.client.util to oej.client

  • Moved implementation classes to oej.client.internal

  • Moved transports to oej.client.transport

  • Moved transport implementation classes to oej.client.transport.internal

  • Moved FastCGI transport classes to o.e.j.fcgi.transport

  • Moved FastCGI transport implementation classes to o.e.j.fcgi.transport.internal

  • Updated WebSocket core client to use only exported, non-internal, oej.client classes.

  • Expanded oej.client.Destination APIs:

    • added: getOrigin(), isSecure(), getProxy(), getConnectionPool(), getHttpClient(), send(..).
    • removed: getScheme(), getHost(), getPort() because they don't uniquely identify a Destination anymore (Origin does)
  • Moved destination sweeper functionality from HttpDestination to HttpClient. HttpDestination does not implement close() anymore, now relies on LifeCycle.stop()

  • Moved HttpReceiver.storeCookie() logic to HttpClient.putCookie() to avoid exposing CookieManager.

  • Moved HttpClient.getAcceptEncodingField() to ContentDecoder.Factories

Signed-off-by: Simone Bordet [email protected]

* Moved oej.client.api to oej.client
* Moved oej.client.util to oej.client
* Moved implementation classes to oej.client.internal
* Moved transports to oej.client.transport
* Moved transport implementation classes to oej.client.transport.internal

* Moved FastCGI transport classes to o.e.j.fcgi.transport
* Moved FastCGI transport implementation classes to o.e.j.fcgi.transport.internal

* Updated WebSocket core client to use only exported, non-internal, oej.client classes.

* Expanded oej.client.Destination APIs:
  - added: getOrigin(), isSecure(), getProxy(), getConnectionPool(), getHttpClient(), send(..).
  - removed: getScheme(), getHost(), getPort() because they don't uniquely identify a Destination anymore (Origin does)
* Moved destination sweeper functionality from HttpDestination to HttpClient.
  HttpDestination does not implement close() anymore, now relies on LifeCycle.stop()
* Moved HttpReceiver.storeCookie() logic to HttpClient.putCookie() to avoid exposing CookieManager.
* Moved HttpClient.getAcceptEncodingField() to ContentDecoder.Factories

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Copy link
Contributor Author

sbordet commented Jan 6, 2023

Sorry for the mega-monster PR, but most of the changes are just changes to import statements.

Notable things to review:

  • Decide whether this is worth, because it'll break all users of HttpClient out there.
    However, it aligns HTTP1, HTTP2, HTTP3 and FCGI transports to the same naming and internalizes all implementation classes, which is a good thing for future maintenance.
    Also, resolves the historical mess created by an initial distinction of classes in the api package, but then other classes considered "api" were added outside of the api package, so it was unclear what was api and what was not.
    Now no more api package, only internal for implementation classes.

  • Websocket core was using what are now "internal" classes, so it was changed to only use exported classes. @lachlan-roberts, you may want to pay particular attention to this.

@gregw
Copy link
Contributor

gregw commented Jan 6, 2023

Is it worthwhile (or possible) to create deprecated classes in the old locations that extend the classes from their new locations? Or does that just prolong the porting hassles?

@sbordet
Copy link
Contributor Author

sbordet commented Jan 6, 2023

Is it worthwhile (or possible) to create deprecated classes in the old locations that extend the classes from their new locations? Or does that just prolong the porting hassles?

It'll be duplicate names everywhere, e.g. oej.client.Request and oej.client.api.Request, etc.

I'm afraid it's going to be really messy.

Certainly oej.client.Destination has incompatible method changes, same for HttpClient.getContentDecoderFactories(), possibly other classes.

And then with maybe 20+ deprecated classes, the question is when we're going to get rid of them?

We can do a good job at documenting the migration though.

@gregw
Copy link
Contributor

gregw commented Jan 7, 2023

Perhaps a conversion script? It at least a list of regex to apply to make the main changes?

@sbordet
Copy link
Contributor Author

sbordet commented Jan 8, 2023

Perhaps a conversion script? It at least a list of regex to apply to make the main changes?

We can create a "Migration Map" file for IntelliJ, so that people would import it in their IDE, run it (via Refactor/Migrate Packages and Classes) and the IDE will update the sources accordingly.

We can save this file in jetty-documentation/src/main/<somewhere>, and reference it from the documentation's migration guide.

Moved TunnelRequest to oej.client.internal.
Avoid public/protected Logger instances.

Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
@@ -61,7 +61,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public abstract class CoreClientUpgradeRequest extends HttpRequest implements Response.CompleteListener, HttpUpgrader.Factory
public abstract class CoreClientUpgradeRequest implements Response.CompleteListener, HttpUpgrader.Factory
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this implement Request or extend something like Request.Wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lachlan-roberts it was extending HttpRequest, but now HttpRequest is an internal class.
The only reason to extend was to also implement HttpUpgrader.Factory, and there was an instanceof check with special logic.
Now the HttpUpgrader.Factory is added as attribute to the request, so there is no strict need for this class to extend Request (which has a million methods to implement).

If you feel you want to give to this class the full Request API, then we can make it implement Request, but then we will have to reimplement all the methods.

I tried to export the internals of jetty-client to this module, but then I had horrible failures at OSGi level, which apparently it is not able to understand JPMS's export ... to ..., so the Import-Package directives were all lacking the necessary internal classes.
For this reason, I went for this current implementation, which feels better because it has a very minimal dependency on jetty-client classes (and works with OSGi).

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed some of the methods have slightly different signature to the ones on Request, for example timeout(long value, TimeUnit unit) return value is different.

But if this class extends a Request.Wrapper (doesn't exist but is easy to generate), then it simplifies this class and stops it diverging from any further changes in the Request interface.

Copy link
Contributor Author

@sbordet sbordet Jan 9, 2023

Choose a reason for hiding this comment

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

I'll give it a go. There is the problem of the covariant return type, but I it's always been there.

The problem is that the implementation often casts down to HttpRequest, so offering a public API Request.Wrapper opens up for either ClassCastExceptions or for a major rewrite of the implementation to avoid casts.

I could probably have a private, maybe just in jetty-websocket-core, RequestWrapper class, or make CoreClientUpgradeRequest implement request, but it's a lot of code that currently is not used (only a few methods are), and it'll require changes when Request changes anyway.
I'm not opposed to have CoreClientUpgradeRequest implements Request, but I think that a has-a relationship is better than a is-a relationship for this case.
Your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also worry that we could miss some of the API from Request that people would like to use on this.
Maybe we could just add a getRequest() method to access the Request?
But either way I think the has-a relationship will work for now.

@sbordet
Copy link
Contributor Author

sbordet commented Jan 10, 2023

@gregw @lorban @lachlan-roberts, these are the classes to review.
All the others have changes in import statements only, or trivial changes due to package names or method signatures.

oej.client.internal.HttpDestination
oej.client.internal.HttpReceiver
oej.client.internal.HttpRequest
oej.client.transport.internal.HttpConnectionOverHTTP
oej.client.transport.internal.ProtocolHttpUpgrader
oej.client.transport.HttpClientTransportDynamic
oej.client.transport.HttpClientTransportOverHTTP
oej.client.AbstractConnectionPool
oej.client.ConnectionPool
oej.client.ContentDecoder
oej.client.ContentResponse
oej.client.Destination
oej.client.DuplexConnectionPool
oej.client.HttpClient
oej.client.HttpClientTransport
oej.client.HttpProxy
oej.client.HttpUpgrader
oej.client.LeakTrackingConnectionPool
oej.client.MultiplexConnectionPool
oej.client.RandomConnectionPool
oej.client.RoundRobinConnectionPool
oej.client.Socks4Proxy
oej.client.Synchronizable
oej.client.TimeoutCompleteListener
oej.client.ValidatingConnectionPool
jetty-client's module-info.java

oej.ee9.websocket.client.impl.DelegatedJettyClientUpgradeRequest
oej.ee9.websocket.client.JettyUpgradeListener

oej.ee10.websocket.client.impl.DelegatedJettyClientUpgradeRequest
oej.ee10.websocket.client.JettyUpgradeListener

oej.fcgi.client.transport.HttpClientTransportOverFCGI

oej.http2.client.transport.internal.HttpConnectionOverHTTP2
oej.http2.client.transport.internal.HttpSenderOverHTTP2
oej.http2.client.transport.HttpClientTransportOverHTTP2

oej.http3.client.transport.internal.HttpSenderOverHTTP3
oej.http3.client.transport.HttpClientTransportOverHTTP3

oej.websocket.core.client.internal.HttpUpgraderOverHTTP2
oej.websocket.core.client.CoreClientUpgradeRequest
oej.websocket.core.client.UpgradeListener

Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

LGTM. Since you haven't modified much at the implementation level I assume that if the build passes this change should be pretty safe.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Nothing bad seen

@sbordet sbordet merged commit a1c5cef into jetty-12.0.x Jan 11, 2023
@sbordet sbordet deleted the fix/jetty-12-internalize-client branch January 11, 2023 09:05
@sbordet sbordet linked an issue Jan 13, 2023 that may be closed by this pull request
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this pull request Jan 16, 2023
…x-document-modules

* upstream/jetty-12.0.x:
  Issue jetty#9167 - making assumption in flaky test
  jetty 12.0.x cleanup duplicate osgi pom metadata (jetty#9093)
  Jetty 12 - Add tests in util/resource for alternate FileSystem implementations (jetty#9149)
  Cleanup non-retainable `Retainable`s (jetty#9159)
  Fixes retainability of special Chunks (jetty#9073)
  TCK: Dispatch forward and includes attributes do not meet the spec (jetty#9074)
  re-enable h3 tests (jetty#8773)
  More fundamental test case
  Reorganization of jetty-client classes. (jetty#9127)
  Removing @disabled from SslUploadTest
  Removing @disabled from jetty-start
  jetty#9134 added test
  ee10: DefaultServlet: Replace checks for isStreaming() by !isWriting()
  jetty#9078 make HttpContent.getByteBuffer() implementations return new ByteBuffer instances and document that contract
  Fixes jetty#9141 - Thread-safe Content.Chunk#slice (jetty#9142)
  Remove `@Disabled` from `jetty-jmx` (jetty#9143)
  Bump maven.version from 3.8.6 to 3.8.7
  Bump maven.version from 3.8.6 to 3.8.7
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this pull request Jan 16, 2023
… into jetty-12.0.x-documentation-lowercase-jetty_home-attribute

* 'jetty-12.0.x' of https://github.com/eclipse/jetty.project:
  Issue jetty#9167 - making assumption in flaky test
  jetty 12.0.x cleanup duplicate osgi pom metadata (jetty#9093)
  Jetty 12 - Add tests in util/resource for alternate FileSystem implementations (jetty#9149)
  Cleanup non-retainable `Retainable`s (jetty#9159)
  Fixes retainability of special Chunks (jetty#9073)
  TCK: Dispatch forward and includes attributes do not meet the spec (jetty#9074)
  re-enable h3 tests (jetty#8773)
  More fundamental test case
  Reorganization of jetty-client classes. (jetty#9127)
  Removing @disabled from SslUploadTest
  Removing @disabled from jetty-start
  Bump maven.version from 3.8.6 to 3.8.7
  Bump maven.version from 3.8.6 to 3.8.7
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.

Jetty 12 - Internalize jetty-client classes
4 participants