search

clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Stabilize tests #673

Closed arnaudgeiser closed 1 year ago

arnaudgeiser commented 1 year ago

Description

Ensure non keep-alived connections are used in multipart and ring integration tests.

KingMob commented 1 year ago

How does this stabilize tests? If we rely on closing the conn instead of marking the final chunk, we have a bug, yeah? And if we can't use a persistent connection reliably, we have a different bug 😄

I don't mind specific tests to check for connection closing and HTTP/1.0 compatibility with Connection: keep-alive/Connection: close, but HTTP/1.0 is almost non-existent these days, and 1.1 assumes keep-alive is the default, so disabling keep-alive seems like it should be rare.

arnaudgeiser commented 1 year ago

How does this stabilize tests? If we rely on closing the conn instead of marking the final chunk, we have a bug, yeah? And if we can't use a persistent connection reliably, we have a different bug smile

Yes, we definitely have some bugs with the client. (c.f the discussion on Slack with Ferdinand, where a connection is currently in use and might be closed before/in between)

The thing is... in integration tests, we are starting/restarting the server multiple times while the client is a shared instance. When a server is closed (too fast? - shutdown-timeout 0), I observed it might have some impacts on the connection with the client, hence with I propose to not reuse the connection in integration tests.

I'm currently working on this issue [1] which is related to those [2]. While it might not be the culprit, I have the hunch it might be kind of related, at least, I'm able to reproduce similar behavior (closed connection, connection stuck). But I need more time to come up with a proposition/PR here...

[1] : https://github.com/clj-commons/aleph/issues/454 [2] : https://github.com/clj-commons/aleph/issues/453

KingMob commented 1 year ago

If you think this change will help those other issues, maybe incorporate it into that future PR instead?

arnaudgeiser commented 1 year ago

I will close it. Let's see if we are able to improve the client behavior then (no lead for now).