Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client premature close (abort) not detected #217

Closed
Tomas2D opened this issue Nov 9, 2024 · 6 comments
Closed

Client premature close (abort) not detected #217

Tomas2D opened this issue Nov 9, 2024 · 6 comments

Comments

@Tomas2D
Copy link
Contributor

Tomas2D commented Nov 9, 2024

Detection of client's premature close has been fixed by #107, but lately broken again via ba26b94.

Tomas2D added a commit to Tomas2D/node-server that referenced this issue Nov 9, 2024
Ref: honojs#217
Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>
@Tomas2D Tomas2D changed the title Client premature close not detected Client premature close (abort) not detected Nov 9, 2024
@usualoma
Copy link
Member

usualoma commented Nov 9, 2024

Hi @Tomas2D
Thank you for creating the issue.

We are aware that the current node-server may not detect connection disconnections in some cases. However, as mentioned in #172, incoming.destroyed also becomes true for a normal POST request, so we are not using it in the node server.

It would be good if we could detect that the connection was really aborted, excluding the case of a normal POST request.

@Tomas2D
Copy link
Contributor Author

Tomas2D commented Nov 10, 2024

Based on nodejs/node#46666 we could do this instead.

outgoing.on('close', () => {
  if (incoming.errored) {
    req[getAbortController]().abort(incoming.errored.toString())
   } else if (!outgoing.writableFinished) {
    req[getAbortController]().abort('Client connection prematurely closed.')
   }
})

@usualoma
Copy link
Member

@Tomas2D Thanks for the reply!
I've confirmed that using outgoing.writableFinished does indeed give the expected result.

Could you please update #218 with this information? We would also appreciate it if you could add a test. If adding a test is difficult, we can handle it here.

Tomas2D added a commit to Tomas2D/node-server that referenced this issue Nov 11, 2024
Ref: honojs#217
Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>
@Tomas2D
Copy link
Contributor Author

Tomas2D commented Nov 11, 2024

Great. PR has been updated. I updated tests also to cover non-GET HTTP requests.

Tomas2D added a commit to Tomas2D/node-server that referenced this issue Nov 11, 2024
Ref: honojs#217
Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>
Tomas2D added a commit to Tomas2D/node-server that referenced this issue Nov 12, 2024
Ref: honojs#217
Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>
yusukebe pushed a commit that referenced this issue Nov 12, 2024
* fix: detection of client premature close

Ref: #217
Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>

* fix: improve detection of client premature close

Ref: #217
Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>

* fixup! fix: improve detection of client premature close

Ref: #217
Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>

* fixup! fixup! fix: improve detection of client premature close

Ref: #217
Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>

---------

Signed-off-by: Tomas Dvorak <toomas2d@gmail.com>
@yusukebe
Copy link
Member

@Tomas2D

Can we close this issue now?

@Tomas2D
Copy link
Contributor Author

Tomas2D commented Nov 12, 2024

Sure. Thank you.

@Tomas2D Tomas2D closed this as completed Nov 12, 2024
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

No branches or pull requests

3 participants