The Trunk: WebClient-Core-pre.117.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

The Trunk: WebClient-Core-pre.117.mcz

commits-2
Patrick Rein uploaded a new version of WebClient-Core to project The Trunk:
http://source.squeak.org/trunk/WebClient-Core-pre.117.mcz

==================== Summary ====================

Name: WebClient-Core-pre.117
Author: pre
Time: 17 September 2018, 4:01:07.501347 pm
UUID: 7908aa6f-cf8c-dd40-b6f3-6efb2b8042d1
Ancestors: WebClient-Core-ul.116

Fixes an issue with chunked encoded WebMessages which resulted in already decoded content to be decoded again. While not perfect it is important to allow for larger GitHub repositories such as Metacello.

=============== Diff against WebClient-Core-ul.116 ===============

Item was added:
+ ----- Method: WebMessage>>streamDirectlyFrom:to:size:progress: (in category 'streaming') -----
+ streamDirectlyFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock
+ "Stream the content of srcStream to dstStream.
+ If a size is given, stream that many elements, otherwise stream up to the end."
+
+ | buffer remaining size totalRead |
+ sizeOrNil = 0 ifTrue:[^self].
+
+ buffer := (srcStream isBinary ifTrue:[ByteArray] ifFalse:[String]) new: 4096.
+ totalRead := 0.
+ size := sizeOrNil ifNil:[0].
+ [(sizeOrNil == nil and:[srcStream atEnd not]) or:[totalRead < size]] whileTrue:[
+ progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].
+ remaining := sizeOrNil ifNil:[99999] ifNotNil:[sizeOrNil - totalRead].
+ remaining > buffer size ifTrue:[remaining := buffer size].
+ buffer := srcStream next: remaining into: buffer startingAt: 1.
+ dstStream nextPutAll: (remaining < buffer size  
+ ifTrue:[(buffer copyFrom: 1 to: remaining)]
+ ifFalse:[buffer]).
+ totalRead := totalRead + buffer size.
+ ].
+ dstStream flush.
+ progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].!

Item was changed:
  ----- Method: WebMessage>>streamFrom:to:size:progress: (in category 'streaming') -----
  streamFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock
+ "Stream the content of srcStream to dstStream applying any conversations necessary."
- "Stream the content of srcStream to dstStream.
- If a size is given, stream that many elements, otherwise stream up to the end."
 
- | buffer totalRead remaining size |
  (self headerAt: 'transfer-encoding') ifNotEmpty:[:encoding|
  encoding = 'chunked'
+ ifTrue:[
+ self flag: #todo. " Ideally this would use the WebChunkedStream --pre"
+ ^self chunkFrom: srcStream to: dstStream progress: progressBlock]
- ifTrue:[^self chunkFrom: srcStream to: dstStream progress: progressBlock]
  ifFalse:[self error: 'Unknown transfer-encoding: ', encoding]].
 
+ ^ self streamDirectlyFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock.!
- sizeOrNil = 0 ifTrue:[^self].
-
- buffer := (srcStream isBinary ifTrue:[ByteArray] ifFalse:[String]) new: 4096.
- totalRead := 0.
- size := sizeOrNil ifNil:[0].
- [(sizeOrNil == nil and:[stream atEnd not]) or:[totalRead < size]] whileTrue:[
- progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].
- remaining := sizeOrNil ifNil:[99999] ifNotNil:[sizeOrNil - totalRead].
- remaining > buffer size ifTrue:[remaining := buffer size].
- buffer := srcStream next: remaining into: buffer startingAt: 1.
- dstStream nextPutAll: (remaining < buffer size  
- ifTrue:[(buffer copyFrom: 1 to: remaining)]
- ifFalse:[buffer]).
- totalRead := totalRead + buffer size.
- ].
- dstStream flush.
- progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].!

Item was changed:
  ----- Method: WebMessage>>streamTo:size:progress: (in category 'streaming') -----
  streamTo: dstStream size: size progress: aBlock
  "Stream from the receiver's socket stream to the given destination stream.
  Inbound. Can be used on both request/response depending on
  whether it is utilized by WebClient or WebServer."
  content ifNil:[
  self streamFrom: stream to: dstStream size: size progress: aBlock
  ] ifNotNil:[
+ self streamDirectlyFrom: content readStream to: dstStream size: size progress: aBlock
- self streamFrom: content readStream to: dstStream size: size progress: aBlock
  ].!


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: WebClient-Core-pre.117.mcz

Tobias Pape
Great!

Question to all: can we please backport to 5.1, at least? or even below?

Best regards
        -Tobias

> On 17.09.2018, at 17:02, [hidden email] wrote:
>
> Patrick Rein uploaded a new version of WebClient-Core to project The Trunk:
> http://source.squeak.org/trunk/WebClient-Core-pre.117.mcz
>
> ==================== Summary ====================
>
> Name: WebClient-Core-pre.117
> Author: pre
> Time: 17 September 2018, 4:01:07.501347 pm
> UUID: 7908aa6f-cf8c-dd40-b6f3-6efb2b8042d1
> Ancestors: WebClient-Core-ul.116
>
> Fixes an issue with chunked encoded WebMessages which resulted in already decoded content to be decoded again. While not perfect it is important to allow for larger GitHub repositories such as Metacello.
>
> =============== Diff against WebClient-Core-ul.116 ===============
>
> Item was added:
> + ----- Method: WebMessage>>streamDirectlyFrom:to:size:progress: (in category 'streaming') -----
> + streamDirectlyFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock
> + "Stream the content of srcStream to dstStream.
> + If a size is given, stream that many elements, otherwise stream up to the end."
> +
> + | buffer remaining size totalRead |
> + sizeOrNil = 0 ifTrue:[^self].
> +
> + buffer := (srcStream isBinary ifTrue:[ByteArray] ifFalse:[String]) new: 4096.
> + totalRead := 0.
> + size := sizeOrNil ifNil:[0].
> + [(sizeOrNil == nil and:[srcStream atEnd not]) or:[totalRead < size]] whileTrue:[
> + progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].
> + remaining := sizeOrNil ifNil:[99999] ifNotNil:[sizeOrNil - totalRead].
> + remaining > buffer size ifTrue:[remaining := buffer size].
> + buffer := srcStream next: remaining into: buffer startingAt: 1.
> + dstStream nextPutAll: (remaining < buffer size  
> + ifTrue:[(buffer copyFrom: 1 to: remaining)]
> + ifFalse:[buffer]).
> + totalRead := totalRead + buffer size.
> + ].
> + dstStream flush.
> + progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].!
>
> Item was changed:
>  ----- Method: WebMessage>>streamFrom:to:size:progress: (in category 'streaming') -----
>  streamFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock
> + "Stream the content of srcStream to dstStream applying any conversations necessary."
> - "Stream the content of srcStream to dstStream.
> - If a size is given, stream that many elements, otherwise stream up to the end."
>
> - | buffer totalRead remaining size |
>   (self headerAt: 'transfer-encoding') ifNotEmpty:[:encoding|
>   encoding = 'chunked'
> + ifTrue:[
> + self flag: #todo. " Ideally this would use the WebChunkedStream --pre"
> + ^self chunkFrom: srcStream to: dstStream progress: progressBlock]
> - ifTrue:[^self chunkFrom: srcStream to: dstStream progress: progressBlock]
>   ifFalse:[self error: 'Unknown transfer-encoding: ', encoding]].
>
> + ^ self streamDirectlyFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock.!
> - sizeOrNil = 0 ifTrue:[^self].
> -
> - buffer := (srcStream isBinary ifTrue:[ByteArray] ifFalse:[String]) new: 4096.
> - totalRead := 0.
> - size := sizeOrNil ifNil:[0].
> - [(sizeOrNil == nil and:[stream atEnd not]) or:[totalRead < size]] whileTrue:[
> - progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].
> - remaining := sizeOrNil ifNil:[99999] ifNotNil:[sizeOrNil - totalRead].
> - remaining > buffer size ifTrue:[remaining := buffer size].
> - buffer := srcStream next: remaining into: buffer startingAt: 1.
> - dstStream nextPutAll: (remaining < buffer size  
> - ifTrue:[(buffer copyFrom: 1 to: remaining)]
> - ifFalse:[buffer]).
> - totalRead := totalRead + buffer size.
> - ].
> - dstStream flush.
> - progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].!
>
> Item was changed:
>  ----- Method: WebMessage>>streamTo:size:progress: (in category 'streaming') -----
>  streamTo: dstStream size: size progress: aBlock
>   "Stream from the receiver's socket stream to the given destination stream.
>   Inbound. Can be used on both request/response depending on
>   whether it is utilized by WebClient or WebServer."
>   content ifNil:[
>   self streamFrom: stream to: dstStream size: size progress: aBlock
>   ] ifNotNil:[
> + self streamDirectlyFrom: content readStream to: dstStream size: size progress: aBlock
> - self streamFrom: content readStream to: dstStream size: size progress: aBlock
>   ].!
>
>


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: WebClient-Core-pre.117.mcz

Levente Uzonyi
In reply to this post by commits-2
On Mon, 17 Sep 2018, [hidden email] wrote:

> Patrick Rein uploaded a new version of WebClient-Core to project The Trunk:
> http://source.squeak.org/trunk/WebClient-Core-pre.117.mcz
>
> ==================== Summary ====================
>
> Name: WebClient-Core-pre.117
> Author: pre
> Time: 17 September 2018, 4:01:07.501347 pm
> UUID: 7908aa6f-cf8c-dd40-b6f3-6efb2b8042d1
> Ancestors: WebClient-Core-ul.116
>
> Fixes an issue with chunked encoded WebMessages which resulted in already decoded content to be decoded again. While not perfect it is important to allow for larger GitHub repositories such as Metacello.
>
> =============== Diff against WebClient-Core-ul.116 ===============
>
> Item was added:
> + ----- Method: WebMessage>>streamDirectlyFrom:to:size:progress: (in category 'streaming') -----
> + streamDirectlyFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock
> + "Stream the content of srcStream to dstStream.
> + If a size is given, stream that many elements, otherwise stream up to the end."
> +
> + | buffer remaining size totalRead |
> + sizeOrNil = 0 ifTrue:[^self].
> +
> + buffer := (srcStream isBinary ifTrue:[ByteArray] ifFalse:[String]) new: 4096.
> + totalRead := 0.
> + size := sizeOrNil ifNil:[0].
> + [(sizeOrNil == nil and:[srcStream atEnd not]) or:[totalRead < size]] whileTrue:[
> + progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].
> + remaining := sizeOrNil ifNil:[99999] ifNotNil:[sizeOrNil - totalRead].
> + remaining > buffer size ifTrue:[remaining := buffer size].
> + buffer := srcStream next: remaining into: buffer startingAt: 1.
> + dstStream nextPutAll: (remaining < buffer size
> + ifTrue:[(buffer copyFrom: 1 to: remaining)]
> + ifFalse:[buffer]).
> + totalRead := totalRead + buffer size.
> + ].
> + dstStream flush.
> + progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].!
>
> Item was changed:
>  ----- Method: WebMessage>>streamFrom:to:size:progress: (in category 'streaming') -----
>  streamFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock
> + "Stream the content of srcStream to dstStream applying any conversations necessary."
> - "Stream the content of srcStream to dstStream.
> - If a size is given, stream that many elements, otherwise stream up to the end."
>
> - | buffer totalRead remaining size |
>   (self headerAt: 'transfer-encoding') ifNotEmpty:[:encoding|
>   encoding = 'chunked'
> + ifTrue:[
> + self flag: #todo. " Ideally this would use the WebChunkedStream --pre"
> + ^self chunkFrom: srcStream to: dstStream progress: progressBlock]
> - ifTrue:[^self chunkFrom: srcStream to: dstStream progress: progressBlock]
>   ifFalse:[self error: 'Unknown transfer-encoding: ', encoding]].
>
> + ^ self streamDirectlyFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock.!
> - sizeOrNil = 0 ifTrue:[^self].
> -
> - buffer := (srcStream isBinary ifTrue:[ByteArray] ifFalse:[String]) new: 4096.
> - totalRead := 0.
> - size := sizeOrNil ifNil:[0].
> - [(sizeOrNil == nil and:[stream atEnd not]) or:[totalRead < size]] whileTrue:[
> - progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].
> - remaining := sizeOrNil ifNil:[99999] ifNotNil:[sizeOrNil - totalRead].
> - remaining > buffer size ifTrue:[remaining := buffer size].
> - buffer := srcStream next: remaining into: buffer startingAt: 1.
> - dstStream nextPutAll: (remaining < buffer size
> - ifTrue:[(buffer copyFrom: 1 to: remaining)]
> - ifFalse:[buffer]).
> - totalRead := totalRead + buffer size.
> - ].
> - dstStream flush.
> - progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].!
>
> Item was changed:
>  ----- Method: WebMessage>>streamTo:size:progress: (in category 'streaming') -----
>  streamTo: dstStream size: size progress: aBlock
>   "Stream from the receiver's socket stream to the given destination stream.
>   Inbound. Can be used on both request/response depending on
>   whether it is utilized by WebClient or WebServer."
>   content ifNil:[
>   self streamFrom: stream to: dstStream size: size progress: aBlock
>   ] ifNotNil:[
> + self streamDirectlyFrom: content readStream to: dstStream size: size progress: aBlock
> - self streamFrom: content readStream to: dstStream size: size progress: aBlock

It seems to me that this is the only real change. And if I'm not
mistaken, that could simply be written as

  dstStream nextPutAll: content

Which would be simpler and way more efficient.

Levente

>   ].!

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: WebClient-Core-pre.117.mcz

David T. Lewis
In reply to this post by Tobias Pape
On Mon, Sep 17, 2018 at 07:48:30PM +0200, Tobias Pape wrote:
> Great!
>
> Question to all: can we please backport to 5.1, at least? or even below?
>
> Best regards
> -Tobias

Sure, that would be a good thing to do. But see Levente's reply before doing
the backport, as there may be a simpler way to do this.

Dave


> > On 17.09.2018, at 17:02, [hidden email] wrote:
> >
> > Patrick Rein uploaded a new version of WebClient-Core to project The Trunk:
> > http://source.squeak.org/trunk/WebClient-Core-pre.117.mcz
> >
> > ==================== Summary ====================
> >
> > Name: WebClient-Core-pre.117
> > Author: pre
> > Time: 17 September 2018, 4:01:07.501347 pm
> > UUID: 7908aa6f-cf8c-dd40-b6f3-6efb2b8042d1
> > Ancestors: WebClient-Core-ul.116
> >
> > Fixes an issue with chunked encoded WebMessages which resulted in already decoded content to be decoded again. While not perfect it is important to allow for larger GitHub repositories such as Metacello.
> >
> > =============== Diff against WebClient-Core-ul.116 ===============
> >
> > Item was added:
> > + ----- Method: WebMessage>>streamDirectlyFrom:to:size:progress: (in category 'streaming') -----
> > + streamDirectlyFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock
> > + "Stream the content of srcStream to dstStream.
> > + If a size is given, stream that many elements, otherwise stream up to the end."
> > +
> > + | buffer remaining size totalRead |
> > + sizeOrNil = 0 ifTrue:[^self].
> > +
> > + buffer := (srcStream isBinary ifTrue:[ByteArray] ifFalse:[String]) new: 4096.
> > + totalRead := 0.
> > + size := sizeOrNil ifNil:[0].
> > + [(sizeOrNil == nil and:[srcStream atEnd not]) or:[totalRead < size]] whileTrue:[
> > + progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].
> > + remaining := sizeOrNil ifNil:[99999] ifNotNil:[sizeOrNil - totalRead].
> > + remaining > buffer size ifTrue:[remaining := buffer size].
> > + buffer := srcStream next: remaining into: buffer startingAt: 1.
> > + dstStream nextPutAll: (remaining < buffer size  
> > + ifTrue:[(buffer copyFrom: 1 to: remaining)]
> > + ifFalse:[buffer]).
> > + totalRead := totalRead + buffer size.
> > + ].
> > + dstStream flush.
> > + progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].!
> >
> > Item was changed:
> >  ----- Method: WebMessage>>streamFrom:to:size:progress: (in category 'streaming') -----
> >  streamFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock
> > + "Stream the content of srcStream to dstStream applying any conversations necessary."
> > - "Stream the content of srcStream to dstStream.
> > - If a size is given, stream that many elements, otherwise stream up to the end."
> >
> > - | buffer totalRead remaining size |
> >   (self headerAt: 'transfer-encoding') ifNotEmpty:[:encoding|
> >   encoding = 'chunked'
> > + ifTrue:[
> > + self flag: #todo. " Ideally this would use the WebChunkedStream --pre"
> > + ^self chunkFrom: srcStream to: dstStream progress: progressBlock]
> > - ifTrue:[^self chunkFrom: srcStream to: dstStream progress: progressBlock]
> >   ifFalse:[self error: 'Unknown transfer-encoding: ', encoding]].
> >
> > + ^ self streamDirectlyFrom: srcStream to: dstStream size: sizeOrNil progress: progressBlock.!
> > - sizeOrNil = 0 ifTrue:[^self].
> > -
> > - buffer := (srcStream isBinary ifTrue:[ByteArray] ifFalse:[String]) new: 4096.
> > - totalRead := 0.
> > - size := sizeOrNil ifNil:[0].
> > - [(sizeOrNil == nil and:[stream atEnd not]) or:[totalRead < size]] whileTrue:[
> > - progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].
> > - remaining := sizeOrNil ifNil:[99999] ifNotNil:[sizeOrNil - totalRead].
> > - remaining > buffer size ifTrue:[remaining := buffer size].
> > - buffer := srcStream next: remaining into: buffer startingAt: 1.
> > - dstStream nextPutAll: (remaining < buffer size  
> > - ifTrue:[(buffer copyFrom: 1 to: remaining)]
> > - ifFalse:[buffer]).
> > - totalRead := totalRead + buffer size.
> > - ].
> > - dstStream flush.
> > - progressBlock ifNotNil:[progressBlock value: sizeOrNil value: totalRead].!
> >
> > Item was changed:
> >  ----- Method: WebMessage>>streamTo:size:progress: (in category 'streaming') -----
> >  streamTo: dstStream size: size progress: aBlock
> >   "Stream from the receiver's socket stream to the given destination stream.
> >   Inbound. Can be used on both request/response depending on
> >   whether it is utilized by WebClient or WebServer."
> >   content ifNil:[
> >   self streamFrom: stream to: dstStream size: size progress: aBlock
> >   ] ifNotNil:[
> > + self streamDirectlyFrom: content readStream to: dstStream size: size progress: aBlock
> > - self streamFrom: content readStream to: dstStream size: size progress: aBlock
> >   ].!
> >
> >
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: WebClient-Core-pre.117.mcz

patrick.rein
In reply to this post by Levente Uzonyi
>> + self streamDirectlyFrom: content readStream to: dstStream size: size progress: aBlock
>> - self streamFrom: content readStream to: dstStream size: size progress: aBlock
>
>It seems to me that this is the only real change. And if I'm not
>mistaken, that could simply be written as
>
> dstStream nextPutAll: content
>
>Which would be simpler and way more efficient.
>

Yes, absolutely. My rational for that solution was to change as little as possible of the current control flow. After digging through various parts of the streamFrom:to: protocol (together with Tobias) we figured out that there are various inconsistent uses of the methods, in particular as WebChunkedStream exists but is not used. Thus, in order to not violate any assumptions of potential clients, I tried to preserve the control flow and added the flag for a potential refactoring after the 5.2 release.

That being said and after looking at the streamDirectlyFrom:to:size:progress: message again carefully: I will change it to your proposed version :) It might take till the European afternoon though for me to upload it.

Bests
Patrick

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: WebClient-Core-pre.117.mcz

Levente Uzonyi
In reply to this post by Levente Uzonyi
Hi Patrick,

One more thing to consider is uploads. While these methods are
currently not used by WebRequest, it is possible to do so, and in such
case my suggestion would remove the progress indicator, which is something
we don't want.
So, I suggest to push this as-is, and review these methods after the
release.

Levente

On Tue, 18 Sep 2018, [hidden email] wrote:

>>> + self streamDirectlyFrom: content readStream to: dstStream size: size progress: aBlock
>>> - self streamFrom: content readStream to: dstStream size: size progress: aBlock
>>
>> It seems to me that this is the only real change. And if I'm not
>> mistaken, that could simply be written as
>>
>> dstStream nextPutAll: content
>>
>> Which would be simpler and way more efficient.
>>
>
> Yes, absolutely. My rational for that solution was to change as little as possible of the current control flow. After digging through various parts of the streamFrom:to: protocol (together with Tobias) we figured out that there are various inconsistent uses of the methods, in particular as WebChunkedStream exists but is not used. Thus, in order to not violate any assumptions of potential clients, I tried to preserve the control flow and added the flag for a potential refactoring after the 5.2 release.
>
> That being said and after looking at the streamDirectlyFrom:to:size:progress: message again carefully: I will change it to your proposed version :) It might take till the European afternoon though for me to upload it.
>
> Bests
> Patrick