Skip to content

Rewrite CloseableHttpResponse to ClassicHttpResponse and use executeOpen#210

Open
shahzebs wants to merge 1 commit intomainfrom
http-response-open
Open

Rewrite CloseableHttpResponse to ClassicHttpResponse and use executeOpen#210
shahzebs wants to merge 1 commit intomainfrom
http-response-open

Conversation

@shahzebs
Copy link
Contributor

@shahzebs shahzebs commented Jan 9, 2026

When using CloseableHttpResponse and execute() with a response handler, the http connection was closed after the response handler terminated. Returning the http response was not feasible because the http connection was closed and thus so was the InputStream with the content of the response.

Using executeOpen() keeps the connection open and makes it possible to return the response object for later use. The InputStream remains open and unconsumed. The caller, howevever, is now responsible for closing the InputStream and by extension also the Http connection. This is easily done as pointed out in the examples with a try-with-resources.

When using CloseableHttpResponse and execute() with a response handler,
the http connection was closed after the response handler terminated.
Returning the http response was not feasible because the http connection
was closed and thus so was the InputStream with the content of the
response.

Using executeOpen() keeps the connection open and makes it possible to
return the response object for later use. The InputStream remains open
and unconsumed. The caller, howevever, is now responsible for closing
the InputStream and by extension also the Http connection. This is
easily done as pointed out in the examples with a try-with-resources.
@shahzebs shahzebs requested a review from eivinhb January 9, 2026 14:45
@shahzebs
Copy link
Contributor Author

shahzebs commented Jan 9, 2026

Should fix #209

@shahzebs shahzebs requested review from a team and runeflobakk January 12, 2026 12:57
@runeflobakk
Copy link
Member

Synes det er vanskelig å mene noe om dette er safe eller ikke. Jeg har ikke nok oversikt over hele API-flaten (som er ganske stor) denne klienten tilbyr nå. Det jeg legger merke til er at det nå virker som alle requests nå bruker executeOpen? Så lenge vi har kontroll på at vi alltid lukker ting internt der biblioteket selv konsumerer responsen, så er det vel i prinsippet ok.

Jeg tror jeg selv ville foretrukket å konsekvent bruke .executeOpen(..) kun de stedene vi skal laste ned en datastrøm (f.eks. et dokument eller noe), da de ulike execute(..)-metoden er laget nettopp for å unngå å gjøre tabber med å glemme IO-håndtering/lukking.

Bare nevner også at å endre returtypen til en metode er en ikke-binærkompatibel endring, sånn med tanke på versjonering. Dersom noe kode et eller annet sted er kompilert mot tidligere variant av en metode, så vil det kræsje hvis denne metoden får en ny versjon av biblioteket.

@eivinhb
Copy link
Member

eivinhb commented Feb 5, 2026

Jeg tror at denne endringen er ok å legge ut i v17, fordi den gjør "ferdig" overgangen til HttpClient5 som vel var årsaken til at 17 ble en major release. Men vi bør kanskje skrive noe om det i versjonen-informasjonen.
Metoden stopSharing er vel den eneste som lekker CloseableHttpResponse ut fra apiet. Det jo være en feil? Jeg kan ikke forstå hvorfor den metoden lekker internals ut på den måten.
Kan du se på det, @shahzebs ?

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 this pull request may close these issues.

3 participants