HTTPClient needs to be shutdown after use to avoid memory leak #4
Conversation
|
Fixes #3 |
Examples/PrintPDF/main.swift
Outdated
| @@ -48,3 +50,5 @@ while true { | |||
|
|
|||
| try await Task.sleep(nanoseconds: 3_000_000_000) | |||
| } | |||
|
|
|||
| try httpClient.syncShutdown() | |||
There was a problem hiding this comment.
This is never executed. The while loop above only exits...
The thing is that is technically completely unnecessary to shutdown the client before terminating anyway (so I did not want to include it in the example) - but having the debug build assertion sure is not nice either...
Also, since we are in an async context, I think this would result in a warning too, right? (we'd have to use .shutdown().get() or something I guess)
There was a problem hiding this comment.
Absolutely right, I fixed the spots.
.syncShutdown() doesn't return anything, it throws if an error occurs. So I think we are good with that call.
Not really recommended programming, but it is to demonstrate the shutdown of the HTTPClient before terminating the program.
Quite right. It's always a struggle with examples, as they are for demonstration and not really high quality code. |
There was a problem hiding this comment.
I don't want to be too pedantic on a silly little example like this, but I think it reads less cleanly than before.
Would you mind refactoring the while-loop to work without exit and have a single shutdown.
Maybe just wrap whole thing in a function that returns properly?
What do you think, is this better? |
|
ping @sliemeobn |
|
sorry, I did not notice this last week. if that helps with the getting a proper error out instead of an asserting, it looks much better, yes. thanks! one tiny last nit: could you remove the comment? it's just not really true that not shutting down causes a memory leak in an app like this - the process terminates anyway. even the NIO team are saying how this is a bit misunderstood. |
I actually would prefer not to remove it. The comment is not really meaningful in this example, I agree, however an important detail for someone using the library for the first time. Otherwise they might miss that part. What did the NIO team say about closing the client? From what I remember it was explicitly documented there that it needs to be closed. |
|
I hope you don't mind, but I think i fixed it for the better. thank you for your patience. |
See https://github.com/swift-server/async-http-client/blob/main/README.md#request-response-api