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

Switch to Java11 HttpClient #205

Closed
laeubi opened this issue Dec 16, 2022 · 24 comments · Fixed by #206
Closed

Switch to Java11 HttpClient #205

laeubi opened this issue Dec 16, 2022 · 24 comments · Fixed by #206

Comments

@laeubi
Copy link
Member

laeubi commented Dec 16, 2022

ECF now includes the Java 11 HttpClient (see eclipse/ecf#12 (comment))

We should include this as early as possible to have enough time to test and get feedback and/or regressions.

I could see two possible options:

  1. we only include Java 11, if there is any issue users can de-install it and install Apache Client5 instead, but it might be problematic if it totally fails
  2. we additionally include Java 11, if there are issues users can de-install it and should be back at Apache Client5, but there is some risk that the presence of Apache Client5 hides some issues we won't see

FYI @akurtakov what do you think? Maybe (2) for 2023-03 and switch to (1) in 2023-06?

@akurtakov
Copy link
Member

I'm off till end of year starting today. In previous cases (e.g. httpclient 3->4, 4->5) Platform went with just including latest and it being default. If there is issue by the time of M2, revert to httpclient 5 can be done.

@vogella
Copy link
Contributor

vogella commented Dec 16, 2022

Cool

laeubi added a commit to eclipse-platform/eclipse.platform.releng.aggregator that referenced this issue Dec 17, 2022
@vogella
Copy link
Contributor

vogella commented Dec 19, 2022

If possible switch to the new provider early M1 to find issues with it. IMHO it would be a nice win to get rid of the httpclient dependency.

@laeubi
Copy link
Member Author

laeubi commented Dec 19, 2022

It should now be available in the target platform, so if you like, just exchange the features and open a PR so we can see what happens :-)

@vogella
Copy link
Contributor

vogella commented Dec 19, 2022

It should now be available in the target platform, so if you like, just exchange the features and open a PR so we can see what happens :-)

I leave that to the more involved people like you or Alex. Would be strange to me to change a feature without having worked / looked at the actual change.

@laeubi
Copy link
Member Author

laeubi commented Dec 19, 2022

akurtakov pushed a commit to laeubi/p2 that referenced this issue Jan 17, 2023
@laeubi
Copy link
Member Author

laeubi commented Jan 24, 2023

I think I have prepared all that is required for "platform" here, Tycho already uses a custom transport for this so if someone is interested in bring this forward it would be good to take over.

@akurtakov
Copy link
Member

I hope to try it out soon(ish). But others are more than welcome to try themself and report issues.

@vogella
Copy link
Contributor

vogella commented Jan 31, 2023

I see that Tycho 4 will start using this. Why not merge this one and revert if we see issue with it?

@laeubi
Copy link
Member Author

laeubi commented Jan 31, 2023

Tycho 4 is using an own transport independent from P2... last time I checked P2 test failed, so someone is needed to look into this:

laeubi added a commit to laeubi/p2 that referenced this issue Jan 31, 2023
laeubi added a commit to laeubi/p2 that referenced this issue Mar 31, 2023
laeubi added a commit to laeubi/p2 that referenced this issue Mar 31, 2023
laeubi added a commit to laeubi/p2 that referenced this issue Jun 7, 2023
laeubi added a commit to laeubi/p2 that referenced this issue Jun 8, 2023
HannesWell pushed a commit to laeubi/p2 that referenced this issue Jun 16, 2023
laeubi added a commit to laeubi/p2 that referenced this issue Sep 18, 2023
laeubi added a commit to laeubi/p2 that referenced this issue Sep 19, 2023
laeubi added a commit to laeubi/p2 that referenced this issue Sep 19, 2023
laeubi added a commit to laeubi/p2 that referenced this issue Sep 19, 2023
@akurtakov
Copy link
Member

Now that this is in an I-build , will java11 httpclient be used instead of apache hc one if both providers are installed ?

@mickaelistria
Copy link
Contributor

I've just updated my SDK to latest I-Build and I see that the httpclient5 ECF connector was removed as part of the update.

@iloveeclipse
Copy link
Member

I've just updated my SDK to latest I-Build and I see that the httpclient5 ECF connector was removed as part of the update.

Could it be, it is not included in the SDK package?
I only see

  • org.apache.httpcomponents.client5.httpclient5.source_5.1.3.v20221013-1742.jar
  • org.eclipse.ecf.provider.filetransfer.httpclient5.source_1.1.0.v20230423-0417.jar
  • org.eclipse.ecf.provider.filetransfer.httpclientjava_2.0.0.v20230715-1752.jar

Shouldn't there be some org.eclipse.ecf.provider.filetransfer.httpclient5 non-source jar?

@iloveeclipse iloveeclipse reopened this Sep 19, 2023
@akurtakov
Copy link
Member

Actually the httpclient5.source ones would better be gone one we are sure of the implementation.

@merks
Copy link
Contributor

merks commented Sep 19, 2023

There seem to be many references to the older things:

image

@merks
Copy link
Contributor

merks commented Sep 19, 2023

I would try to help more, but I really need to get a new version of ant (1.10.14) ready ASAP...

@akurtakov
Copy link
Member

I've done installation and it works fine but having people behind weird proxies test would be much appreciated.

@merks
Copy link
Contributor

merks commented Sep 19, 2023

Yes, I tested the installer and stuff too a few months ago. It all seems fine. But weird networks with firewalls and proxies with authentication are my biggest concern...

Perhaps @sratz is familiar with such environments and has time to try it out...

@mickaelistria
Copy link
Contributor

I've just updated my SDK to latest I-Build and I see that the httpclient5 ECF connector was removed as part of the update.

It's actually not an issue for me, but more the confirmation that the installation and replacement process is going fine. We do not want httpclient5 anymore, but httpclientjava (which is provided with the update).
I would suggest to close this issue; and open new ones to get rid of remaining references to httpclient5.

@sratz
Copy link
Contributor

sratz commented Sep 19, 2023

I know that httpclient5-based ECF comes with a special org.eclipse.ecf.provider.filetransfer.httpclient5.win32 bundle specifically for NTLM and SPNEGO proxy authentication. I have no idea how well these environments are supported by the Java 11 HTTP client (I also don't have access to such environments to test it).

EGit offers a choice to switch between the HTTP client implementations:
image

Could be an alternative to allow for a phased transition and to have a fallback?

@merks
Copy link
Contributor

merks commented Sep 19, 2023

Even if such a thing is feasible, no one will actually invest effort to implement that. 😱

@laeubi
Copy link
Member Author

laeubi commented Sep 20, 2023

Maintaining two implementations is actually the opposite of what the goal here is.
Nerveless every user with special demands can uninstall the java11 http feature and install any alternative implementation, its just a matter of adding the ECF update site (maybe @merks might be gracious to help enhancing the p2.inf of these features that installing one implementation automatically remove the others) I think that's fair enough.

merks added a commit to eclipse-oomph/oomph that referenced this issue Sep 20, 2023
@merks
Copy link
Contributor

merks commented Sep 20, 2023

The following might not determine the winner, but it does ensure that org.eclipse.ecf.filetransfer.httpclientjava.feature is a contender for the title:

<import feature="org.eclipse.ecf.filetransfer.httpclientjava.feature" version="2.0.0" match="compatible"/>

FYI, I've been able to get the installer fully working with the new implementation, including in Oomph's ECFURIHandlerImpl.

I think we could no proceed with the remaining cleanup...

merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Sep 23, 2023
merks added a commit to merks/p2 that referenced this issue Sep 23, 2023
merks added a commit that referenced this issue Sep 23, 2023
merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Sep 23, 2023
merks added a commit to eclipse-platform/eclipse.platform.releng.aggregator that referenced this issue Sep 23, 2023
@merks
Copy link
Contributor

merks commented Sep 23, 2023

I believe the cleanup is done.

A new problem with authentication is tracked by this new issue:

#330

@merks merks closed this as completed Sep 23, 2023
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 a pull request may close this issue.

7 participants