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

Package "stream" is misleadingly named #9

Closed
tv42 opened this issue Jan 15, 2015 · 9 comments
Closed

Package "stream" is misleadingly named #9

tv42 opened this issue Jan 15, 2015 · 9 comments

Comments

@tv42
Copy link

tv42 commented Jan 15, 2015

github.com/mailgun/oxy/stream seems to do the opposite of streaming. "Reads the entire request and response into buffer, [...]"

Maybe it should really be named buffer?

@klizhentas
Copy link
Contributor

great catch, I thought about, buffer is too generic though, I promise to rename it if you come up with a great name :-)

@bradgignac
Copy link

StreamBuffer

@klizhentas
Copy link
Contributor

@bradgignac too long

@klizhentas
Copy link
Contributor

streambuf?

@jeremyschlatter
Copy link
Contributor

oxybuf?

@tv42
Copy link
Author

tv42 commented Jan 16, 2015

retry? gather?

@John-Appleseed
Copy link

buffer/buffer.go - Buffer request into memory or disk

Reads the entire request and response into buffer, optionally buffering it to disk for large requests.
Checks the buffer.Mem and buffer.Max for the requests and responses, rejecting in case if the buffer size was exceeded.
Changes request content-transfer-encoding from chunked and provides total buffer size to the handlers.


Ex.

buffer up to 2MB in memory

buffer.MemRequestBodyBytes(2 * 1024 * 1024)

buffer sounds great! when I'm reading the code as buffer.

code shift for readability

  • stream limits to buffer size

haven't read all the code yet, but I will PR the shift in code if sounds good in use

archisgore pushed a commit that referenced this issue Jun 7, 2016
This handles the issue: #9

It is a better name and helps clarify what the intent and function of this
object is.
mathpl pushed a commit to mathpl/oxy that referenced this issue Aug 18, 2016
Parse the MIME media type from the Content-Type header
archisgore pushed a commit that referenced this issue Aug 30, 2016
This handles the issue: #9

It is a better name and helps clarify what the intent and function of this
object is.
archisgore pushed a commit that referenced this issue Aug 30, 2016
This handles the issue: #9

It is a better name and helps clarify what the intent and function of this
object is.
archisgore pushed a commit that referenced this issue Aug 30, 2016
…#45)

* Renamed Stream to Buffer

This handles the issue: #9

It is a better name and helps clarify what the intent and function of this
object is.

* Forked the original stream into Buffer.

* Fixed a build break

* Simplified stream to be pass-through for now.

* Most UTs for stream expect StatusOk as return code

* More updates towards allowing streaming

1. Initial run at the feedback received on the code review, namely,
   updated all comments/docs to say "buffer" in the buffer part of the
   code, and fixed the mistaken package "steam" to "stream"

2. In fwd.go, replaced RoundTripper with ReverseProxy when
   the stream flag is provided. Not sure yet ReverseProxy
   won't buffer and wait for EOF to flush bytes down to the client
   but having a httpStreamingForwarded as a playground, ensures
   the working version of the forwarded isn't affected by any bugs/mistakes
   I introduce.

* Finished up the httpStreamingForwarder.

* Externalize FlushInterval, with a default, and add a URL connect/disconnect listener.

* Make URL to listener, a pointer so listeners can track them correctly.

* Go fmt

* Updated documentation for stream

* Rename Stream test correctly.

* Rebased against master

* Merge artifact removed

* Added a chunked transfer encoding unit test.
@lunemec
Copy link

lunemec commented Jan 19, 2017

@archisgore renamed the package, maybe this could be closed?

@archisgore
Copy link

I'm closing it. Please reopen if something is amiss.

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

7 participants