Upgrade to OCaml 5 and EIO #134
Conversation
WalkthroughThe changes update various package dependencies and module interfaces across the codebase. In Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PgxEio as pgx_eio.connect
participant IO as pgx_eio.Io
participant Conn as Connection Manager
Client->>PgxEio: Call connect(~net, ~sw, options)
PgxEio->>IO: Initiate IO operations (open connection, SSL upgrade, etc.)
IO->>Conn: Establish connection and manage resources
Conn-->>PgxEio: Return connection instance
PgxEio-->>Client: Connection ready
sequenceDiagram
participant Client
participant Mirage as pgx_lwt_mirage.connect
participant TCP as TCP Stack
participant HE as Happy_eyeballs_mirage (H)
Client->>Mirage: Call connect with (STACK, H)
Mirage->>TCP: Initiate TCP connection
Mirage->>HE: Use Happy Eyeballs for enhanced connectivity
TCP-->>Mirage: Return low-level connection
HE-->>Mirage: Confirm connectivity parameters
Mirage-->>Client: Return module implementing Pgx_lwt.S
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
dune-project(2 hunks)pgx/test/test_pgx_value.ml(3 hunks)pgx_async.opam(1 hunks)pgx_async/bin/pgx_async_example.ml(1 hunks)pgx_async/src/dune(1 hunks)pgx_async/src/pgx_async.ml(4 hunks)pgx_async/src/pgx_async_test.ml(1 hunks)pgx_eio.opam(1 hunks)pgx_eio/src/dune(1 hunks)pgx_eio/src/pgx_eio.ml(1 hunks)pgx_eio/src/pgx_eio.mli(1 hunks)pgx_eio/test/dune(1 hunks)pgx_eio/test/test_pgx_eio.ml(1 hunks)pgx_lwt_mirage/src/pgx_lwt_mirage.ml(4 hunks)pgx_lwt_mirage/src/pgx_lwt_mirage.mli(1 hunks)pgx_test/src/pgx_test.ml(2 hunks)pgx_unix/src/dune(1 hunks)pgx_value_core/src/pgx_value_core.ml(3 hunks)pgx_value_core/src/pgx_value_core.mli(2 hunks)pgx_value_core/test/test_pgx_value_core.ml(2 hunks)
🔇 Additional comments (27)
pgx/test/test_pgx_value.ml (2)
6-6: LGTM! Good choice for UUID generation.The implementation correctly uses
Uuidm.v4_genwith a self-initializing random state, which is suitable for test scenarios.
173-173: LGTM! Consistent usage of the new UUID generator.The changes consistently use the new
uuid_v4function across test cases, improving code reuse.Also applies to: 201-201
pgx_test/src/pgx_test.ml (1)
399-399: LGTM! Consistent usage of the new UUID generator.The change consistently uses the new
uuid_v4function, improving code reuse.pgx_unix/src/dune (1)
13-13: LGTM! Addition of unix dependency aligns with OCaml 5 upgrade.The addition of the
unixlibrary dependency is appropriate for Unix-specific functionality.Let's verify the impact on dependent packages:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for packages that depend on pgx_unix to ensure they're compatible with the new dependency # Search for dependencies on pgx_unix in dune files rg -l "pgx_unix" --type dune # Search for dependencies in opam files rg -l "pgx_unix" --type opamLength of output: 115
🏁 Script executed:
#!/bin/bash # Revised search for dependencies on pgx_unix in dune and opam files using glob patterns echo "Searching for pgx_unix in *.dune files:" rg -l "pgx_unix" --iglob "*.dune" echo "Searching for pgx_unix in *.opam files:" rg -l "pgx_unix" --iglob "*.opam"Length of output: 394
🏁 Script executed:
#!/bin/bash # This script lists all dune and opam files and then searches for "pgx_unix" within them echo "Listing all .dune files in the repository:" fd -e dune || echo "No dune files found." echo -e "\nSearching for 'pgx_unix' in all .dune files:" fd -e dune -x grep -H "pgx_unix" {} || echo "No occurrences of 'pgx_unix' found in dune files." echo -e "\nListing all .opam files in the repository:" fd -e opam || echo "No opam files found." echo -e "\nSearching for 'pgx_unix' in all .opam files:" fd -e opam -x grep -H "pgx_unix" {} || echo "No occurrences of 'pgx_unix' found in opam files."Length of output: 777
LGTM!
The addition of theunixlibrary dependency in thepgx_unix/src/dunefile aligns with the OCaml 5 upgrade and does not appear to negatively impact any dependent packages. Searches through the repository’s dune and opam files did not reveal any external references topgx_unixthat might be affected by this change.pgx_lwt_mirage/src/pgx_lwt_mirage.mli (1)
21-28: Verify downstream usage of the updated module signature.By adding the new RANDOM parameter as Mirage_crypto_rng_mirage.S and introducing the H parameter of type Happy_eyeballs_mirage.S, any existing references to connect and random APIs may need to be updated outside this file. Also, confirm that the new signature for connect (STACK.t * H.t) is consistently applied wherever connections are established, to avoid type mismatch issues.
Suggestion:
Use a shell script to recursively search for connect calls referencing the old signature:Then confirm each of these references has been updated to match the new (STACK.t * H.t) signature.
✅ Verification successful
🏁 Script executed:
#!/bin/bash rg "connect\s*\(\s*[^,\)]+\)" -A 2Length of output: 242
Downstream Usage of Updated Signature Looks Consistent
Based on the grep results, no calls using the outdated (single-argument) form of connect were detected. The only occurrence—“Conduit_async.connect (`Unix_domain_socket path)” in pgx_async/src/pgx_async.ml—appears to refer to a different module and is not indicative of a mismatch regarding the new (STACK.t * H.t) signature in pgx_lwt_mirage.mli.
• No downstream references were found that still invoke connect with a single argument, suggesting that the updated interface is consistently applied.
• The Conduit_async.connect usage in pgx_async does not conflict with the updated signature in pgx_lwt_mirage.pgx_eio/src/pgx_eio.ml (2)
26-48: Be cautious with the custom protect pattern.Raising Exit inside f and re-catching it for cleanup is somewhat unconventional. While it ensures finally is called, it might complicate debugging if other parts of the code raise Exit intentionally. Test carefully to confirm that your finalizers always run as expected and do not inadvertently intercept or swallow exceptions.
Would you like a script to scan for any user-defined raise Exit occurrences across the codebase?
213-253: Confirm coverage for prepared statements and concurrency cases.The prepared statement module is comprehensive, but verifying concurrency behavior (e.g., multiple fibers using the same prepared statement) is essential. Ensure that any locking or synchronization happens correctly, especially if the same statement is shared among concurrent flows.
Could you run concurrency tests or share logs that confirm no data races occur when statements are executed from different fibers?
pgx_eio/test/dune (1)
1-4: Test configuration looks good.The test stanza correctly references eio_main, pgx_test, and pgx_eio. Verify that the tests cover both basic and advanced scenarios, including concurrency, connection errors, and prepared statements.
pgx_eio/src/dune (1)
1-15: LGTM! Well-structured dune configuration.The configuration correctly:
- Handles preprocessing for code coverage using bisect_ppx
- Includes all necessary dependencies (eio, eio_main, pgx, unix) for the new pgx_eio package
pgx_async/src/dune (1)
14-14: LGTM! Dependencies updated correctly.The addition of
async_logaligns with the PR objectives for upgrading dependencies.pgx_value_core/src/pgx_value_core.mli (1)
1-14: LGTM! Time handling updated consistently.The migration from
Time.ttoTime_float.tis applied consistently across all time-related functions. Let's verify the impact of this change.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining Time.t usages that might need updating # Search for Time.t usages rg "Time\.t" --type ocaml # Search for potential time-related function calls that might need updating rg "Time\.(to_string|of_string|now|add|sub|diff|compare)" --type ocamlLength of output: 100
I'm going to run an additional shell script to re-verify that all time-related types are updated correctly and no outdated references to Time.t remain in the repository.
🏁 Script executed:
#!/bin/bash # Verify that the pgx_value_core module correctly uses Time_float.t for time values echo "---- Checking pgx_value_core/src/pgx_value_core.mli (first 30 lines) ----" sed -n '1,30p' pgx_value_core/src/pgx_value_core.mli echo "---- Searching repository for any usage of 'Time.t' ----" rg "Time\.t" --type ocaml echo "---- Searching repository for usage of 'Time_float.t' ----" rg "Time_float\.t" --type ocamlLength of output: 1418
Time Migration Consistency Verified
- pgx_value_core/src/pgx_value_core.mli correctly uses Time_float.t in all time-related functions.
- A repository-wide search confirmed that no outdated Time.t references remain.
- Other references to Time_float.t (in the implementation and tests) align with this change.
pgx_async.opam (1)
14-16: Verify compatibility of async packages with OCaml 5.The upgrade to v0.17.0 for async packages is significant. Let's verify compatibility with OCaml 5.
✅ Verification successful
🌐 Web query:
What is the latest version of async_kernel and async_unix that supports OCaml 5?💡 Result:
The latest versions of `async_kernel` and `async_unix` supporting OCaml 5 are **v0.17.x** (specifically v0.17.1 for full compatibility). Here's the breakdown: ### Key Findings 1. **Jane Street v0.17 Release** - All packages (except `sexplib0`) now target OCaml 5.1+[2][9] - Fixes for OCaml 5.2 compatibility were added in `core.v0.17.1`[2] 2. **Current Versions** ```markdown - async_kernel: v0.17.x [7][9] - async_unix: v0.17.x [8][10] ``` 3. **OCaml Version Support** | Package | OCaml 5 Support | Notes | |--------------|-----------------|--------------------------------| | async_kernel | 5.1+ | Requires core ≥v0.17[2][9] | | async_unix | 5.1+ | Depends on updated scheduler[8] | The v0.17 series represents Jane Street's first major release series with full OCaml 5 support[2][9]. Earlier versions (v0.16.x) only supported OCaml ≤4.x[4][7]. Citations: - 1: https://ocaml.janestreet.com/ocaml-core/CHANGES.html - 2: https://discuss.ocaml.org/t/ann-v0-17-release-of-jane-street-packages/14717 - 3: https://lwn.net/Articles/616326/ - 4: https://discuss.ocaml.org/t/opam-insists-on-downgrades-because-of-ocamlfind/389 - 5: https://github.com/janestreet/async_unix - 6: https://dev.realworldocaml.org/concurrent-programming.html - 7: https://opam.ocaml.org/packages/async_kernel/ - 8: https://ocaml.org/p/async_unix/latest/CHANGES.md.html - 9: https://ocaml.org/p/async_kernel/v0.17.0 - 10: https://opam.ocaml.org/packages/async_unix/
Async Packages Compatibility Confirmed
The async packages in the v0.17 series (with, for example, async_kernel v0.17.1 and async_unix v0.17.1) fully support OCaml 5. Since the pgx_async.opam file specifies a lower bound of "v0.17.0", it is correctly set up for OCaml 5 compatibility.pgx_eio/src/pgx_eio.mli (2)
22-26: LGTM! Clean module type definition.The module type
Scorrectly extendsPgx.Swith the type constraint'a Io.t = 'a, which is essential for direct-style IO with Eio.
42-53: LGTM! Resource-safe connection handling.The
with_connfunction provides a safe way to handle database connections, ensuring proper resource cleanup.pgx_value_core/test/test_pgx_value_core.ml (2)
1-1: LGTM! Consistent time module migration.The migration from
TimetoTime_floatis consistently applied across printer and testable definitions.Also applies to: 5-8
14-20: LGTM! Comprehensive time format testing.The test cases thoroughly cover various time formats and edge cases:
- Full timestamp with milliseconds
- Timestamps without milliseconds
- Different timezone formats (UTC, hour-only, full)
- Roundtrip conversions
Also applies to: 22-28, 30-35
pgx_value_core/src/pgx_value_core.ml (2)
1-1: LGTM! Correct time module migration.The migration to
Time_floatis correctly implemented, maintaining timezone handling behavior.Also applies to: 21-21
24-56: LGTM! Well-documented time string handling.The time string parsing logic is thoroughly documented and handles various formats:
- Timestamps without timezone
- UTC timestamps
- Local timezone timestamps
- Timestamps with/without milliseconds
dune-project (2)
81-86: LGTM! Dependencies updated to support newer Async versions.The update to Async dependencies (async_log, async_kernel, async_unix) to v0.17.0 aligns with the codebase changes and modernizes the async support.
101-110: Verify OCaml version compatibility.The new pgx_eio package requires OCaml 5.00+, which is a significant version requirement. This aligns with the PR objective of upgrading to OCaml 5, but we should ensure all dependencies are compatible with this version.
✅ Verification successful
🌐 Web query:
What is the latest stable version of OCaml, and is OCaml 5.00 considered stable for production use?💡 Result:
The latest stable version of OCaml is **5.3.0**, released on January 8, 2025[1][4][10]. This version includes syntax for deep effect handlers, restored MSVC Windows support, statistical memory profiling improvements, Unicode source file support, and numerous standard library additions[1][4]. Regarding **OCaml 5.00 (5.0.0)** stability for production: - While OCaml 5 introduced revolutionary features like shared-memory parallelism and effect handlers[3], early versions (including 5.x up through at least mid-2024) were considered experimental compared to the **OCaml 4.14 LTS** branch[6][7] - Key stability considerations: | Version | Production Stability | Notable Improvements | |---------|-----------------------|-----------------------| | **4.x** | Fully stable LTS | Security fixes until April 2025[6][7] | | **5.x** | Maturing | Architecture support restored (RISC-V/s390x/Power)[2], performance fixes[8], reduced memory usage[1] | | **5.3** | Recommended | MSVC support restored, statmemprof reintroduced[4] | For production systems requiring absolute stability: - **OCaml 4.14.x remains the LTS choice** until at least April 2025[6][7] - New projects should use **OCaml ≥5.1** with monitoring tools like Olly for observability[2] - Performance-critical applications should benchmark with Sandmark[2] and consider GC configuration[8] The community considers OCaml ≥5.1 production-ready for most workloads[2], though some edge cases still show performance variations compared to OCaml 4[8]. The compiler team maintains rigorous benchmarking and backported allocator testing to ensure stability[2]. Citations: - 1: https://github.com/ocaml/ocaml/releases - 2: https://tarides.com/blog/2023-07-07-making-ocaml-5-succeed-for-developers-and-organisations/ - 3: https://ocaml.org/releases/5.0.0 - 4: https://ocaml.org/releases/5.3.0 - 5: https://discuss.ocaml.org/t/the-road-to-ocaml-5-0/8584 - 6: https://github.com/ocaml/ocaml/blob/trunk/release-info/introduction.md - 7: https://ocaml.org/tools/compiler-release-cycle - 8: https://discuss.ocaml.org/t/ocaml-5-performance/15014 - 9: https://discourse.haskell.org/t/8-months-of-ocaml-after-8-years-of-haskell-in-production/8405 - 10: https://ocaml.org/changelog/2025-01-08-ocaml-5.3.0
OCaml Version Requirement Confirmed
- The pgx_eio package requires OCaml ≥ 5.00, which is aligned with the migration objective to OCaml 5.
- Web results show that the latest stable release is OCaml 5.3.0. While OCaml 5.00 (and early 5.x versions) are maturing, production users are advised to consider 5.1 or later for improved stability.
- All dependencies (e.g., eio, pgx) should be reviewed to confirm compatibility with OCaml 5.x, but the version specification in the dune-project file is appropriate for the upgrade focus.
pgx_lwt_mirage/src/pgx_lwt_mirage.ml (2)
26-33: LGTM! Enhanced security with crypto-safe random number generation.The change from
Mirage_random.StoMirage_crypto_rng_mirage.Simproves security by using cryptographically secure random number generation. The addition ofHappy_eyeballsmodule enhances IPv4/IPv6 dual-stack support.
91-91: Verify DNS and stack handling compatibility.The changes to DNS module instantiation and stack handling reflect the new MirageOS networking architecture. The tuple-based stack handling (
tcp_stack, dns_stack) supports better separation of concerns.Also applies to: 99-102, 115-115
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining old-style stack usage patterns # Search for potential old-style stack usage rg -A 5 "STACK\.t(?!.*\*.*dns_stack)"Length of output: 278
🏁 Script executed:
#!/bin/bash # Description: Re-run check for legacy stack usage patterns using PCRE2, which supports look-around. rg --pcre2 -A 5 "STACK\.t(?!.*\*.*dns_stack)"Length of output: 822
🏁 Script executed:
#!/bin/bash # Search for any reference to "dns_stack" in the repository to see if the new tuple-based stack handling is implemented elsewhere. rg -l "dns_stack"Length of output: 54
🏁 Script executed:
#!/bin/bash # Search for the instantiation of the DNS module to verify it's using the new tuple-based stack handling. rg -n "Dns_client_mirage\.Make"Length of output: 157
DNS and Stack Handling Compatibility Verified
Analysis of the repository confirms that the DNS module instantiation and stack handling are in line with the new tuple-based architecture. In particular:
• The instantiation of the DNS module in pgx_lwt_mirage.ml (line 91) continues to pass STACK correctly, matching the expected new signature.
• The type signatures (as seen in the .mli file) and the presence of references to dns_stack in the file indicate that STACK.t now encapsulates both tcp_stack and dns_stack as per the new design.
• A look for any legacy stack patterns (using the old "STACK.t" style without the tuple) did not reveal problematic instances.Based on these observations, the changes are compatible with the updated MirageOS networking architecture.
pgx_async/src/pgx_async.ml (2)
37-37: LGTM! Updated to use standard library namespace.The change from
Caml.ChartoStdlib.Charaligns with OCaml's standard library modernization.Also applies to: 59-59
101-102: LGTM! Enhanced logging with async-aware implementation.The switch to
Async_log.Globalprovides better integration with Async's runtime, ensuring proper log flushing in asynchronous contexts.pgx_eio.opam (1)
11-17: Verify EIO package compatibility.The dependencies look appropriate for the EIO integration. However, we should verify the EIO package version compatibility with OCaml 5.00+.
pgx_async/src/pgx_async_test.ml (1)
1-1: 🧹 Nitpick (assertive)Module Import Update in Test File
The change from "open Core_kernel" (previously) to "open Core" on line 1 appears to be part of the overall upgrade to OCaml 5 and related dependency updates. Please verify that all functions, types, and utilities previously provided by Core_kernel remain available or are appropriately replaced via Core.pgx_async/bin/pgx_async_example.ml (1)
2-2: 🧹 Nitpick (assertive)Module Import Update in Example File
Changing the import on line 2 from "open Core_kernel" to "open Core" is consistent with the upgrade initiative. Confirm that this broader import doesn’t inadvertently introduce namespace conflicts and that any specialized functions formerly available exclusively in Core_kernel are either not needed or are now accessible via Core.
| let make ~net ~sw = | ||
| let module Thread = struct | ||
| include Io | ||
| type sockaddr = | ||
| | Unix of string | ||
| | Inet of string * int | ||
| type close_flow = unit -> unit | ||
| type in_channel = Eio.Buf_read.t * close_flow | ||
| type out_channel = Eio.Buf_write.t | ||
| let random_list_element lst = | ||
| List.nth lst (Random.int (List.length lst)) | ||
| let open_connection sockaddr = | ||
| let addr : Eio.Net.Sockaddr.stream = | ||
| match sockaddr with | ||
| | Unix path -> `Unix path | ||
| | Inet (hostname, port) -> | ||
| let service = string_of_int port in | ||
| let addrs = Eio.Net.getaddrinfo_stream net hostname ~service in | ||
| random_list_element addrs | ||
| in | ||
| let flow = Eio.Net.connect ~sw net addr in | ||
| let r = Eio.Buf_read.of_flow ~max_size:(1 * 1024 * 1024) flow in | ||
| let close_flow_triggered, resolve_close_flow_triggered = Eio.Promise.create () in | ||
| let w, resolve_w = Eio.Promise.create () in | ||
| Eio.Fiber.fork ~sw begin fun () -> | ||
| Eio.Buf_write.with_flow ~initial_size:(4 * 1024) flow begin fun w -> | ||
| Eio.Promise.resolve resolve_w w; | ||
| Eio.Promise.await close_flow_triggered | ||
| end; | ||
| Eio.Net.close flow | ||
| end; | ||
| let w = Eio.Promise.await w in | ||
| let close_flow () = | ||
| Eio.Promise.resolve resolve_close_flow_triggered () | ||
| in | ||
| (r, close_flow), w | ||
| let upgrade_ssl = `Not_supported |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider error handling and flow termination.
The code spawns a new fiber for writing and eventually closes the flow in line 79, but if net or flow operations fail (e.g., partial writes, abrupt closures), the function might not handle them gracefully. You may want to wrap these operations with error handling to avoid potential resource leaks or unhandled exceptions.
There was a problem hiding this comment.
The new Fiber is mostly a work-around for the unfortunate Eio API situation that there is only a Eio.Buf_write.with_flow but not a Eio.Buf_write.of_flow.
The clean solution would be to switch to Eio.Buf_write.of_flow, once available, for which I already have a running pull request:
There was a problem hiding this comment.
@vog Thank you for the explanation. That makes perfect sense.
✏️ Learnings added
Learnt from: vog
PR: arenadotio/pgx#134
File: pgx_eio/src/pgx_eio.ml:50-86
Timestamp: 2025-02-09T01:13:17.916Z
Learning: The Fiber usage in `pgx_eio` for handling `Eio.Buf_write.with_flow` is a temporary workaround due to missing `Eio.Buf_write.of_flow` functionality in Eio. This will be replaced with direct `of_flow` usage once PR ocaml-multicore/eio#796 is merged.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| val connect : | ||
| net:[> [> `Generic ] Eio.Net.ty ] Eio.Resource.t -> | ||
| sw:Eio.Switch.t -> | ||
| ?host:string -> | ||
| ?port:int -> | ||
| ?user:string -> | ||
| ?password:string -> | ||
| ?database:string -> | ||
| ?unix_domain_socket_dir:string -> | ||
| ?verbose:int -> | ||
| ?max_message_length:int -> | ||
| unit -> | ||
| t |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding documentation for connection parameters.
The connect function provides comprehensive connection options. Consider adding documentation for each parameter to improve usability.
Add OCaml documentation comments:
val connect :
+ (** [connect ~net ~sw ?host ?port ?user ?password ?database ?unix_domain_socket_dir ?verbose ?max_message_length ()]
establishes a new database connection.
@param net The Eio network resource
@param sw The Eio switch for resource management
@param host The database host (default: "localhost")
@param port The database port (default: 5432)
@param user The database user
@param password The database password
@param database The database name
@param unix_domain_socket_dir The Unix domain socket directory
@param verbose The verbosity level
@param max_message_length The maximum message length
*)
net:[> [> `Generic ] Eio.Net.ty ] Eio.Resource.t ->There was a problem hiding this comment.
The level of documentation of this function is consistent with the level of documentation in pgx/src/pgx_intf.ml. It might make sense to add a comment explaining the new parameters net and sw, but then, those are standard Eio parameters that a already explained more than detailed enough in the Eio documentation.
There was a problem hiding this comment.
🧩 Analysis chain
@vog I understand and agree. You're right that we should maintain consistency with pgx_intf.ml's documentation style, and the standard Eio parameters are already well-documented in Eio's documentation. I'll retract my suggestion.
🏁 Script executed:
#!/bin/bash
# Find and show the connect function documentation in pgx_intf.ml
rg -A 10 "val connect" pgx/src/pgx_intf.ml
Length of output: 319
✏️ Learnings added
Learnt from: vog
PR: arenadotio/pgx#134
File: pgx_eio/src/pgx_eio.mli:28-40
Timestamp: 2025-02-09T01:46:51.317Z
Learning: Documentation level in pgx_eio should be consistent with pgx_intf.ml. Standard Eio parameters (net, sw) don't need additional documentation as they are already well-documented in Eio's documentation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
pgx_eio/src/pgx_eio.ml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
pgx_eio/src/pgx_eio.ml (2)
Learnt from: vog
PR: arenadotio/pgx#134
File: pgx_eio/src/pgx_eio.ml:59-60
Timestamp: 2025-02-09T01:17:20.140Z
Learning: In pgx implementations (pgx_unix, pgx_lwt_unix, pgx_eio), Random.int is used for load balancing DNS-resolved addresses, which is an acceptable non-security-critical use case.
Learnt from: vog
PR: arenadotio/pgx#134
File: pgx_eio/src/pgx_eio.ml:50-86
Timestamp: 2025-02-09T01:13:18.060Z
Learning: The Fiber usage in `pgx_eio` for handling `Eio.Buf_write.with_flow` is a temporary workaround due to missing `Eio.Buf_write.of_flow` functionality in Eio. This will be replaced with direct `of_flow` usage once PR ocaml-multicore/eio#796 is merged.
🔇 Additional comments (5)
pgx_eio/src/pgx_eio.ml (5)
1-21: License block looks fine.
No issues here—this is a straightforward license header.
22-23: Module type and external definition are appropriately declared.
Defining "module type S = Pgx.S with type 'a Io.t = 'a" and the external reraise function appear correct and consistent with the rest of the code.
24-48: Io module logic is concise and well-structured.
The “protect” function appropriately ensures that the “finally” block is always called. The approach using “Exit” is a common OCaml pattern, and everything here looks aligned with typical usage.
59-60: Use of Random.int for load balancing is acceptable.
Based on prior discussions and learnings, this non-cryptographic approach is fine for selecting among DNS-resolved addresses in a non-security-critical context.
74-80: Reiterating fiber error-handling concerns.
This section still uses an auxiliary fiber for writing with limited handling of potential I/O failures (e.g., partial writes, abrupt closures). Consider wrapping these operations in more robust error handling to avoid possible resource leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
pgx_eio/src/dune(1 hunks)
🔇 Additional comments (1)
pgx_eio/src/dune (1)
1-8: LGTM! Clean implementation of conditional code coverage instrumentation.The preprocessing configuration correctly handles all cases for the BISECT_ENABLE environment variable, following best practices for optional code coverage instrumentation.
|
@arenadotio I went through all feedback provided by CodeRabbit, explained the current state, and adjusted this PR where it made sense to me. Is this PR now acceptable to you, or is there anything else to do? |
|
@arenadotio In the mirage-channel project it is suggested in mirage/mirage-channel#35 (comment) that Pgx should remove its dependency on mirage-channel-lwt. What is your take on this? |
|
I used to maintain this but no longer work there, not sure if anyone's listening @pedroMMM ? What's the future of this project? |
These commits fix various issues that appeared when compiling PGX with OCaml and the latest versions of the dependency libraries.
Moreover, the last commit introduces
pgx_eiowhich finally adds Eio support for PGX.Summary by CodeRabbit
New Features
Refactor
Tests
Chores