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

HttpServer - autoclose HttpResponse OutputStream on onRequest exit #2094

Closed
DartBot opened this issue Mar 9, 2012 · 6 comments
Closed

HttpServer - autoclose HttpResponse OutputStream on onRequest exit #2094

DartBot opened this issue Mar 9, 2012 · 6 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-as-intended Closed as the reported issue is expected behavior library-io

Comments

@DartBot
Copy link

DartBot commented Mar 9, 2012

This issue was originally filed by Fir...@gmail.com


What steps will reproduce the problem?
void onRequestHandler( HttpRequest httpRequest, HttpResponse httpResponse ) {
  httpResponse.writeString("test");
  httpResponse.outputStream.close();
  print("test");
}
The code above blocks HttpServer response and because of that the browser too.

void onRequestHandler( HttpRequest httpRequest, HttpResponse httpResponse ) {
  httpResponse.writeString("test");
  httpResponse.outputStream.close();
  print("test");
}
Closing the outputstream fixes the problem.

What is the expected output? What do you see instead?
Shouldn't outputstream be closed by HttpServer after exit the onRequest method?

What version of the product are you using? On what operating system?
dart-sdk revision 4978
Mint KDE 12

Please provide any additional information below.

@DartBot
Copy link
Author

DartBot commented Mar 9, 2012

This comment was originally written by ladicek@gmail.com


Shouldn't outputstream be closed by HttpServer after exit the onRequest method?

Most certainly not. This is a little confusing (I think that something like httpResponse.done() would be more instructive and easily discoverable), but the output stream really can't be closed automatically when the onRequest callback finishes. The reason is -- there might be (and probably will be) async operations still running when the callback returns, and they will want to write to the stream. Consider something like this:

httpServer.onRequest = (req, resp) {
  var in = new File("/path/to/very/big/file").openInputStream();
  in.pipe(resp.outputStream);
};

Here, the onRequest callback most probably ends a lot sooner than the whole response is written (streaming a big file to the client), so the HttpServer really can't close the output stream automatically. (As a side note: InputStream.pipe closes the given OutputStream automatically after everything is written, and closing an "exhausted" InputStream isn't necessary. So streaming a file really is this simple, except of error handling -- kudos to the dart:io team!)

@DartBot
Copy link
Author

DartBot commented Mar 9, 2012

This comment was originally written by Firen2k5...@gmail.com


Yes, adding httpResponse.writeDone() method was my second thought.
Any second thoughts about it?

@DartBot
Copy link
Author

DartBot commented Mar 11, 2012

This comment was originally written by ladicek@gmail.com


Actually, something like httpResponse.writeDone() used to be there, but it's long gone. I hope it's coming back in some form, having it in the HttpResponse interface is much more instructive than having a mention about closing the output stream in the documentation comment.

@madsager
Copy link
Contributor

Added Area-IO, Triaged labels.

@madsager
Copy link
Contributor

We cannot automatically close the response at the end of onRequest. For simple examples it would work. However, as Ladislav points out, as soon as you start performing async IO operations to get parts of you response it would break.

Consider:

server.onRequest = (request, response) {
  var f = new File(request.path);
  f.exists((res) {
    if (exists) {
      response.outputStream.writeString("hello");
    } else {
      ...
    }
  });
}

Since the exists call is asynchronous we will exit onRequest immediately without having written anything to response. Therefore, we cannot close it at that point.

Regarding adding shortcuts to HttpResponse itself that will forward to the outputStream. That would be very simple. I personally like the current way with no shortcuts. It is clear what you are doing. You are operating on an output stream.


Added AsDesigned label.

@kevmoo
Copy link
Member

kevmoo commented May 14, 2014

Removed Area-IO label.
Added Area-Library, Library-IO labels.

@DartBot DartBot added Type-Defect area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io closed-as-intended Closed as the reported issue is expected behavior labels May 14, 2014
copybara-service bot pushed a commit that referenced this issue Apr 24, 2023
…n, sse, stream_channel, test, tools, usage, webdev

Revisions updated by `dart tools/rev_sdk_deps.dart`.

async (https://github.com/dart-lang/async/compare/0127813..ce650b0):
  ce650b0  2023-04-19  Nate Bosch  Regression test for error rejecting transaction (#230)

characters (https://github.com/dart-lang/characters/compare/ba8d557..b306414):
  b306414  2023-04-19  Lasse R.H. Nielsen  Add tools to update and generate tables and tests. (#72)

convert (https://github.com/dart-lang/convert/compare/8812e40..855aeac):
  855aeac  2023-04-10  Kevin Moore  Require Dart 2.19, fix override param name, update lints (#81)

crypto (https://github.com/dart-lang/crypto/compare/1cb1528..77491f5):
  77491f5  2023-04-19  Lasse R.H. Nielsen  Make `DigestSink` implement `Sink` (#146)

dartdoc (https://github.com/dart-lang/dartdoc/compare/a0755f5..8e8b36e):
  8e8b36e3  2023-04-19  Janice Collins  Fix up documentation on comment reference parser to align with wiki (#2851)
  b9178fce  2023-04-19  dependabot[bot]  Bump github/codeql-action from 2.2.11 to 2.2.12 (#3390)
  37b981c4  2023-04-19  dependabot[bot]  Bump actions/checkout from 3.5.0 to 3.5.2 (#3389)
  dadef24a  2023-04-10  dependabot[bot]  Bump github/codeql-action from 2.2.9 to 2.2.11 (#3386)
  dda699a7  2023-04-10  Sam Rawlins  Prepare mixin-like classes for Dart 3.0.0 (#3385)

intl (https://github.com/dart-lang/intl/compare/a958db0..5d65e38):
  5d65e38  2023-04-19  Moritz  Update README.md
  9972a89  2023-04-19  Moritz  Update README.md
  5c14faa  2023-04-18  Copybara-Service  Merge pull request #584 from dart-lang:fixDateFormattingUrl
  4ea644e  2023-04-17  Moritz  Merge branch 'master' into fixDateFormattingUrl
  b0d5687  2023-04-17  dependabot[bot]  Bump dart-lang/setup-dart from 1.4.0 to 1.5.0 (#574)
  92149be  2023-04-17  Moritz  Merge branch 'master' into fixDateFormattingUrl
  5203d6e  2023-04-17  Googler  Internal change
  bab667f  2023-04-17  Moritz  Typo
  b0896b1  2023-04-17  Moritz  Fix bug
  43507e1  2023-04-14  Googler  Internal change
  d8844a0  2023-04-14  dependabot[bot]  Bump actions/checkout from 3.3.0 to 3.5.2 (#579)
  99ed16f  2023-04-13  Copybara-Service  Merge pull request #572 from dart-lang:adaptBrowserTest
  34e824c  2023-03-30  Moritz  Adapt test to browser CLDR update

markdown (https://github.com/dart-lang/markdown/compare/d437c85..5f98aea):
  5f98aea  2023-04-20  Jonas Finnemann Jensen  Throw, if `BlockSyntax.parseLines` loops indefinitely (#533)
  86ebc2c  2023-04-19  Zhiguang Chen  Fix `linkReferenceDefinitionPattern` (#532)

sse (https://github.com/dart-lang/sse/compare/11e83a0..f947c3d):
  f947c3d  2023-04-10  Kevin Moore  Require Dart 2.19, bump lints (#82)

stream_channel (https://github.com/dart-lang/stream_channel/compare/74646ea..71d4690):
  71d4690  2023-04-11  Tobe Osakwe  Add example/example.dart (#52)

test (https://github.com/dart-lang/test/compare/7832931..7fab079):
  7fab0792  2023-04-19  Oleh Prypin  Dart 3 compatibility: change `extends Iterator` to `implements Iterator` (#1997)
  8f7682a5  2023-04-18  Nate Bosch  Remove deprecation of test_api top level libraries (#1994)
  7151486c  2023-04-14  Simon Binder  Add support for Microsoft Edge (#1992)
  c1d686aa  2023-04-12  Parker Lougheed  Fix "Improvements" link in `package:checks` migration guide (#1991)

tools (https://github.com/dart-lang/tools/compare/545d7e1..5c9f45c):
  5c9f45c  2023-04-20  Elias Yishak  Move `dateStamp` getter to `utils.dart` (#83)

usage (https://github.com/dart-lang/usage/compare/0698711..f97752f):
  f97752f  2023-04-10  Devon Carew  update readme for deprecation (#192)

webdev (https://github.com/dart-lang/webdev/compare/7546291..8b42c95):
  8b42c950  2023-04-21  Anna Gringauze  Cleanup record types display (#2070)
  1cfb3bd6  2023-04-20  Elliott Brooks  Update CONTRIBUTING docs (#2097)
  941eda5b  2023-04-19  Elliott Brooks  Add a test to make sure proper release procedure is followed for `dwds` and `webdev` (#2095)
  2eb7c3ee  2023-04-17  Elliott Brooks  Add Github workflow for Dart Code Metrics (#2092)
  79a9bc9b  2023-04-17  Elliott Brooks  Fix DCM analyzer warnings (#2094)
  2a625039  2023-04-14  Elliott Brooks  Add `vm_service` git package dependency override (#2078)
  1fa19603  2023-04-12  Elliott Brooks  Add CI check to remind contributor to update CHANGELOG (#2090)
  c7bb19e1  2023-04-12  Elliott Brooks  Prevent PRs labeled `do-not-submit` from being merged (#2088)
  3781ef9b  2023-04-10  Elliott Brooks  Run mono_repo generate with version 6.5.3 (#2084)
  843890c6  2023-04-10  Anna Gringauze  Refactor record shape processing out of calculating record bound fields (#2074)

Change-Id: I4ce65f9f79d5086c33c575e57eff300216392510
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/297800
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Moritz Sümmermann <mosum@google.com>
Commit-Queue: Moritz Sümmermann <mosum@google.com>
copybara-service bot pushed a commit that referenced this issue Sep 21, 2023
Revisions updated by `dart tools/rev_sdk_deps.dart`.

ecosystem (https://github.com/dart-lang/ecosystem/compare/ed39318..dcf5c4f):
  dcf5c4f  2023-09-20  Parker Lougheed  Simplify dart_flutter_team_lints install instructions (#172)

http (https://github.com/dart-lang/http/compare/decefa6..1251619):
  1251619  2023-09-19  Brian Quinlan  Add the ability to control the URL cache. (#1020)

mockito (https://github.com/dart-lang/mockito/compare/412c0be..097e563):
  097e563  2023-09-19  Ilya Yanok  Add a missing dummy `bool` value

shelf (https://github.com/dart-lang/shelf/compare/e2a02b7..4851978):
  4851978  2023-09-20  Kevin Moore  shelf_router_generator: prepare to release v1.1.0 (#380)

test (https://github.com/dart-lang/test/compare/6449495..8191a35):
  8191a355  2023-09-20  Nate Bosch  Drop a TODO about running browser after compile (#2094)
  d8e9d87d  2023-09-18  Nate Bosch  Add a silent reporter (#2093)

tools (https://github.com/dart-lang/tools/compare/70d778d..3c248df):
  3c248df  2023-09-21  Devon Carew  misc infra updates for dart-lang/tools (#165)

Change-Id: I436a34847db75f45a20b8c18996419f88214485f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327280
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-as-intended Closed as the reported issue is expected behavior library-io
Projects
None yet
Development

No branches or pull requests

3 participants