Remove akka from runtime #8953

Akirathan · 2024-02-02T18:05:12Z

Pull Request Description

There are two projects transitively required by runtime, that have akka dependencies:

downloader
connected-lock-manager

This PR replaces the akka-http dependency in downloader by HttpClient from JDK, and splits connected-lock-manager into two projects such that there are no akka classes in runtime.jar.

Important Notes

Simplify the downloader project - remove akka.
Add HTTP tests to the downloader project that uses our http-test-helper that is normally used for stdlib tests.
It required few tweaks so that we can embed that server in a unit test.
Split connected-lock-manager project into two projects - remove akka from runtime.
Native image build fixes and quality of life improvements:
- Output of native-image is captured 743e167
  - The output will no longer be intertwined with the output from other commands on the CI.
- Arguments to the native-image are passed via an argument file, not via command line - ba0a69d
  - This resolves an issue on Windows with "Command line too long", for example in https://github.com/enso-org/enso/actions/runs/7934447148/job/21665456738?pr=8953#step:8:2269

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

hubertp · 2024-02-02T19:49:28Z

Are you aware of #8934? You can probably more or less revert the commit that introduced it.

…oader

Akirathan · 2024-02-09T13:53:28Z

Are you aware of #8934? You can probably more or less revert the commit that introduced it.

Thanks for pointing that out. Let's first try to use only JDK. I think that the HttpClient provided by the JDK is more than capable of handling few requests. There is no need to add yet another dependency.

…oader

Seems like some leftover.

Akirathan · 2024-02-13T15:50:03Z

engine/runtime/src/test/java/org/enso/runtime/test/DependenciesTest.java

+  public void noAkkaDependencies() {
+    try {
+      Class.forName("akka.actor.ActorSystem$");
+      fail("akka.actor.ActorSystem should not be on the classpath");


This test is a bit clumsy. If akka.actor.ActorSystem is not on the class-path, it only means that runtime/test does not have akka dependency, it does not check that there are no akka classes in runtime.jar. However, I still think that this test is useful. Moreover, I have manually checked that runtime.jar does not contain any akka classes.

JaroslavTulach · 2024-02-14T10:30:33Z

lib/scala/downloader/src/main/scala/org/enso/downloader/http/HTTPRequest.scala

@@ -1,6 +1,6 @@
 package org.enso.downloader.http

-import akka.http.scaladsl.model.HttpRequest
+import java.net.http.HttpRequest


Using class from JDK is a great step forward!

radeusgd · 2024-02-15T15:56:10Z

lib/scala/downloader/src/main/java/org/enso/downloader/http/FilesMultipartBodyPublisher.java

+/**
+ * This utility class adds support for {@code multipart/form-data} content-type as there is no such
+ * support in {@code java.net.http} JDK.
+ *
+ * <p>Inspired by <a href="https://stackoverflow.com/a/56482187/4816269">SO</a>.
+ */
+public class FilesMultipartBodyPublisher {
+  public static HttpRequest.BodyPublisher ofMimeMultipartData(


I don't like us implementing such utilities ourselves instead of using existing well-tested solutions.

This works nicely for simple cases, but how do we guarantee that it will work well in general? Should we try testing it as well as an existing solution would? That adds a lot of maintenance burden on us.

Example 1: the boundary.

I see it is generated as a UUID. I guess it is extremely unlikely that the same UUID would appear inside of the contents of one of the parts submitted as part of this form, but it is not 100% guaranteed.

I'm a bit nitpicky here as indeed it is going to be very unlikely to run into this kind of collision. But was this even considered when this code was written?

This is a seemingly simple operation but it may raise many uncertainities like the example above. IMO relying on an existing solution is better as it does not put the maintenance burden of ensuring this solution is bug free, on us.

The downloader project makes only a very specific kind of HTTP requests and expects a very specific and narrow set of HTTP responses. We are in a full control over these reqs and responses. This project is only used for library uploading and downloading, which is also covered by tests.

Implementing a very simple handling of multipart/form-data is a minimal price we have to pay here. Please keep in mind that we are trying very hard, in general, to reduce the number of classes that will eventually get into runtime.jar.

lib/scala/downloader/src/main/scala/org/enso/downloader/http/URIBuilder.scala

radeusgd

Looks good.

I'm a bit worried about rolling our own URI and Multipart body handling - these seem like they are simple, but there are traps hidden there.

But I don't want to be complaining too much - we should be fine with these for now and they are not heavily used anyway.

I'd appreciate if we put a comment for future maybe, so that once we start using these more, we could revisit and see if any 'reinforcements' are needed there.

hubertp · 2024-02-15T16:18:18Z

@radeusgd Didn't you mention that at some point you moved from JDK to Apache because of some bug in the former? Is that fixed now?

radeusgd · 2024-02-15T16:25:05Z

@radeusgd Didn't you mention that at some point you moved from JDK to Apache because of some bug in the former? Is that fixed now?

IIRC that was this bug https://bugs.openjdk.org/browse/JDK-8231449 it seems to indeed be fixed in Java 14 (we've been using Java 11 back then and I think it was never backported to versions <13).

…oader

…oader # Conflicts: # tools/legal-review/engine/report-state # tools/legal-review/launcher/report-state # tools/legal-review/project-manager/report-state

In PR #8953, in commit ba0a69d, I have introduced argument files to the `native-image`. In this PR, let's try to upload these argfiles as artifacts on GH, so that we can inspect them later.

Akirathan added 4 commits February 2, 2024 19:00

Add withDebug commands to some projects

0b38e89

Add MultipartBodyPublisher

9f8832f

URIBuilder uses java.net.URI

0c9719b

Refactor HTTPRequestBuilder to java.net from akka

3688cc8

Akirathan self-assigned this Feb 2, 2024

enso-bot bot mentioned this pull request Feb 2, 2024

Remove akka dependencies from runtime project #8222

Closed

Migrate downloader to JDK HttpClient

bd71fd9

Akirathan force-pushed the wip/akirathan/8222-remove-akka-from-downloader branch from c59bffc to bd71fd9 Compare February 9, 2024 13:46

Akirathan added 3 commits February 9, 2024 14:47

fmt

b17f86b

Merge branch 'develop' into wip/akirathan/8222-remove-akka-from-downl…

8444663

…oader

fmt

0ef2ffd

Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 9, 2024

Akirathan added 14 commits February 12, 2024 11:23

Merge branch 'develop' into wip/akirathan/8222-remove-akka-from-downl…

506c582

…oader

Extract runServer method in HTTPTestHelperServer

dc1e582

downloader uses http-test-helper for testing

7d79c02

Ensure http-test-helper main method is blocked until interrupted

ed8a567

Add some HttpDownloader tests

28173fb

Add bodyhandlers that report HttpReponse progress

4163d29

Add optional logging to simple-library-server tool

a681dc2

Fix IOException in PathProgressBodyHandler

b845057

Fix URIBuilder

3e6f3a4

Simplify posting files

b9cdeea

fmt

0bab2bb

Merge branch 'develop' into wip/akirathan/8222-remove-akka-from-downl…

c746448

…oader

remove unused import

62c1414

remove akka from downloader

ec44437

Akirathan changed the title ~~Remove akka from downloader and from runtime~~ Remove akka from downloader Feb 13, 2024

remove akka from logging-service.

5eaef2e

Seems like some leftover.

Akirathan requested review from JaroslavTulach and hubertp as code owners February 13, 2024 15:47

Akirathan commented Feb 13, 2024

View reviewed changes

Update license review

f3f3606

enso-bot bot mentioned this pull request Feb 13, 2024

Add filter cmdline option to tests #8998

Closed

Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Feb 14, 2024

Change line-endings of some license files

4f1e54c

JaroslavTulach approved these changes Feb 14, 2024

View reviewed changes

Fix compilation of connected-lock-manager/Test

0d30555

radeusgd reviewed Feb 15, 2024

View reviewed changes

lib/scala/downloader/src/main/scala/org/enso/downloader/http/URIBuilder.scala Outdated Show resolved Hide resolved

radeusgd approved these changes Feb 15, 2024

View reviewed changes

Akirathan added 2 commits February 16, 2024 14:10

Merge branch 'develop' into wip/akirathan/8222-remove-akka-from-downl…

0e2b1d6

…oader

URIBuilder handles special characters

c5cc2dc

Akirathan force-pushed the wip/akirathan/8222-remove-akka-from-downloader branch from 891587a to c5cc2dc Compare February 16, 2024 13:20

Akirathan added 3 commits February 16, 2024 14:36

Capture the output from native-image build.

743e167

fmt

6a9fff2

Pass arguments to native-image via argfile

ba0a69d

Akirathan force-pushed the wip/akirathan/8222-remove-akka-from-downloader branch from 3e12343 to ba0a69d Compare February 19, 2024 12:33

Akirathan mentioned this pull request Feb 19, 2024

Upload native-image argfiles #9094

Merged

1 task

Akirathan added 2 commits February 19, 2024 15:48

Merge branch 'develop' into wip/akirathan/8222-remove-akka-from-downl…

148a64d

…oader # Conflicts: # tools/legal-review/engine/report-state # tools/legal-review/launcher/report-state # tools/legal-review/project-manager/report-state

Update report-state in license review

9965c4c

Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Feb 19, 2024

mergify bot merged commit 96082c3 into develop Feb 19, 2024
25 of 29 checks passed

mergify bot deleted the wip/akirathan/8222-remove-akka-from-downloader branch February 19, 2024 16:39

unfurl-links bot mentioned this pull request Feb 27, 2024

HttpDownloaderTest.downloadBigFileWithProgress failing #9193

Closed

Akirathan mentioned this pull request Feb 29, 2024

HTTPDownloader fails to install new engine #9221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove akka from runtime #8953

Remove akka from runtime #8953

Akirathan commented Feb 2, 2024 •

edited

Loading

hubertp commented Feb 2, 2024

Akirathan commented Feb 9, 2024

Akirathan Feb 13, 2024

JaroslavTulach Feb 14, 2024

radeusgd Feb 15, 2024

Akirathan Feb 16, 2024

radeusgd left a comment

hubertp commented Feb 15, 2024

radeusgd commented Feb 15, 2024

Remove akka from runtime #8953

Remove akka from runtime #8953

Conversation

Akirathan commented Feb 2, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

hubertp commented Feb 2, 2024

Akirathan commented Feb 9, 2024

Akirathan Feb 13, 2024

Choose a reason for hiding this comment

JaroslavTulach Feb 14, 2024

Choose a reason for hiding this comment

radeusgd Feb 15, 2024

Choose a reason for hiding this comment

Akirathan Feb 16, 2024

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

hubertp commented Feb 15, 2024

radeusgd commented Feb 15, 2024

Akirathan commented Feb 2, 2024 •

edited

Loading