search

Zaid-Ajaj / Fable.Remoting

Type-safe communication layer (RPC-style) for F# featuring Fable and .NET Apps
https://zaid-ajaj.github.io/Fable.Remoting/
MIT License
273 stars 55 forks source link

Cancel Async if the Request is aborted #115

Closed 0x53A closed 5 years ago

0x53A commented 5 years ago

I have a (relatively) expensive operation in one of my server functions.

If the client cancels the request (because it timed out client-side, or because the user closed the browser) I would like to be notified.

There is a CancellationToken exposed in the HttpContext that fires if the client aborts: https://github.com/aspnet/HttpAbstractions/issues/197#issuecomment-279129671

This should just forward that token to Async.

0x53A commented 5 years ago

My scenario is already possible using Remoting.fromContext instead of Remoting.fromValue, do you think this would make sense anyway?

If yes I'd add something similar to the other entry points (suave, etc) if they also expose a cancellation handler,

0x53A commented 5 years ago

Hm I don't quite understand the travis error:

grafik

Zaid-Ajaj commented 5 years ago

Looks like a good default choice for people using Remoting.fromValue, I like it

If yes I'd add something similar to the other entry points (suave, etc) if they also expose a cancellation handler,

Yes please

Hm I don't quite understand the travis error:

I restarted the build, but anyways seems safe to ignore since all integration tests passed :smile:

Zaid-Ajaj commented 5 years ago

Nope, looks like Travis was right:

Untitled

0x53A commented 5 years ago

yeah, that happened because I was too lazy to clone and edited it blind in the github UI.

So apparently AppVeyor doesn't compile the giraffe stuff.

Will add the other two in the evening

Zaid-Ajaj commented 5 years ago

So apparently AppVeyor doesn't compile the giraffe stuff.

Indeed, AppVeyor only runs the end-to-end tests using a Suave server, I probably should change that

Will add the other two in the evening

Just let me know when this is ready, so I can roll out new packages :smile:

0x53A commented 5 years ago

I couldn't find any ConnectionAborted event for suave, but I'm not familiar with it. /cc @haf

I did add it to the raw AspNetCore server.

Also, I'm a little disappointed, the event fires a bit late, maybe 10 seconds or more after I close the browser window. Still better than nothing, but shouldn't it be able to detect a closed tcp connection "instantly"?

Zaid-Ajaj commented 5 years ago

I couldn't find any ConnectionAborted event for suave, but I'm not familiar with it. /cc @haf

I will take a look

I did add it to the raw AspNetCore server.

Awesome, I will merge and publish Giraffe & AspNetCore adapters first

Also, I'm a little disappointed, the event fires a bit late, maybe 10 seconds or more after I close the browser window. Still better than nothing, but shouldn't it be able to detect a closed tcp connection "instantly"?

Honestly, I have no idea, maybe this is something to file at the Ap.net core repo

0x53A commented 5 years ago

Thanks!

Honestly, I have no idea, maybe this is something to file at the Ap.net core repo

Yes, it was more of a statement than a question. It's still good to have - I had an issue where the requests all went into a queue, the clients timed out but and the queue continued to grow, forcing a hard reboot of the whole server because the CPU was maxed out and we couldn't even connect remotely to restart the process.