search

Enet4 / dicom-rs

Rust implementation of the DICOM standard
https://dicom-rs.github.io
Apache License 2.0
416 stars 81 forks source link

Starting the discussion on async support #476

Open jennydaman opened 6 months ago

jennydaman commented 6 months ago

I see on the roadmap that there is the intention for async support.

https://github.com/Enet4/dicom-rs/wiki/Roadmap#async-api

I am trying to write (what I think is) a SCP, mostly based on the code of storescp/src/main.rs. For each received DICOM file, I need to make an HTTP request, so the program could be much faster if dicom-rs supported async. Also, I would like to be able to use some of my favorite crates, opentelemetry (which prefers tokio) and fs_err (which, for async, requires tokio).

I have identified FileDicomObj::write_to_file and the TcpStream accepting methods of dicom_ul::association::ServerAssociationOptions as befitting of async.

@Enet4 are you currently working on async? If not, how would you like to approach it?

A key question is: do we want to lock ourselves into Tokio?

jennydaman commented 6 months ago

Actually, a bit of Rust circlejerking: I did benchmarks and as-is, the SCP service I wrote is too fast for the Python Django backend it interacts with. So there's no real motivation for me to optimize it further...

Enet4 commented 6 months ago

Thank you for the initiative, @jennydaman. Going async for DICOM network operations is unquestionably the way forward for the project.

I have identified FileDicomObj::write_to_file and the TcpStream accepting methods of dicom_ul::association::ServerAssociationOptions as befitting of async.

You are right that the best place to start with async support would be at the dicom_ul association API, preferably by offering a nonblocking and holding a blocking version for a smoother transition (or in scenarios where this isn't very important). Async file reading and writing could be a subsequent step, but I feel that non-blocking network is more important.

@Enet4 are you currently working on async? If not, how would you like to approach it?

I remember doing some tinkering to find out the implications of making an async version of associations, though I did not have much done here. I do recall that there was a need to rewrite the PDU reading logic, due to the direct use of blocking Read calls everywhere. I am likely only resuming this until after 0.7.0 is released, but if you would like I can push a WIP branch of what I have so others can continue building on top of it.

A key question is: do we want to lock ourselves into Tokio?

Using Tokio is a reasonable approach. The Tokio ecosystem is powerful and mature, making it much easier to build what we want without resorting to complicated low-level constructs.

Actually, a bit of Rust circlejerking: I did benchmarks and as-is, the SCP service I wrote is too fast for the Python Django backend it interacts with. So there's no real motivation for me to optimize it further...

That is fine, it's great to know that the current implementation works for you. :) In any case there is a place for async constructs in other scenarios.

naterichman commented 6 months ago

I started a very rough draft in #463 which only adds the async at the application level, not in the library. To be fair, I haven't done that much async in any language except maybe JS, but I read this blog post the other day and it seems like adding it at the library level is hard